Re: [PATCH] net: sock: simplify tw proto registration

On Wed, Mar 10, 2021 at 1:39 AM Alexander Duyck
[off-list ref] wrote:

On Mon, Mar 8, 2021 at 7:15 PM [off-list ref] wrote:

quoted

From: Tonghao Zhang <redacted>

Introduce a new function twsk_prot_init, inspired by
req_prot_init, to simplify the "proto_register" function.

Signed-off-by: Tonghao Zhang <redacted>
---
 net/core/sock.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 0ed98f20448a..610de4295101 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c

@@ -3475,6 +3475,32 @@ static int req_prot_init(const struct proto *prot)
        return 0;
 }

+static int twsk_prot_init(const struct proto *prot)
+{
+       struct timewait_sock_ops *twsk_prot = prot->twsk_prot;
+
+       if (!twsk_prot)
+               return 0;
+
+       twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s",
+                                             prot->name);
+       if (!twsk_prot->twsk_slab_name)
+               return -ENOMEM;
+
+       twsk_prot->twsk_slab =
+               kmem_cache_create(twsk_prot->twsk_slab_name,
+                                 twsk_prot->twsk_obj_size, 0,
+                                 SLAB_ACCOUNT | prot->slab_flags,
+                                 NULL);
+       if (!twsk_prot->twsk_slab) {
+               pr_crit("%s: Can't create timewait sock SLAB cache!\n",
+                       prot->name);
+               return -ENOMEM;
+       }
+
+       return 0;
+}
+

So one issue here is that you have two returns but they both have the
same error clean-up outside of the function. It might make more sense
to look at freeing the kasprintf if the slab allocation fails and then
using the out_free_request_sock_slab jump label below if the slab
allocation failed.

Hi, thanks for your review.
if twsk_prot_init failed, (kasprintf, or slab alloc), we will invoke
the tw_prot_cleanup() to clean up
the sources allocated.
1. kfree(twsk_prot->twsk_slab_name); // if name is NULL, kfree() will
return directly
2. kmem_cache_destroy(twsk_prot->twsk_slab); // if slab is NULL,
kmem_cache_destroy() will return directly too.
so we don't care what err in twsk_prot_init().

and req_prot_cleanup() will clean up all sources allocated for req_prot_init().

quoted

 int proto_register(struct proto *prot, int alloc_slab)
 {
        int ret = -ENOBUFS;

@@ -3496,22 +3522,8 @@ int proto_register(struct proto *prot, int alloc_slab)
                if (req_prot_init(prot))
                        goto out_free_request_sock_slab;

-               if (prot->twsk_prot != NULL) {
-                       prot->twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", prot->name);
-
-                       if (prot->twsk_prot->twsk_slab_name == NULL)
-                               goto out_free_request_sock_slab;
-
-                       prot->twsk_prot->twsk_slab =
-                               kmem_cache_create(prot->twsk_prot->twsk_slab_name,
-                                                 prot->twsk_prot->twsk_obj_size,
-                                                 0,
-                                                 SLAB_ACCOUNT |
-                                                 prot->slab_flags,
-                                                 NULL);
-                       if (prot->twsk_prot->twsk_slab == NULL)
-                               goto out_free_timewait_sock_slab;
-               }
+               if (twsk_prot_init(prot))
+                       goto out_free_timewait_sock_slab;

So assuming the code above takes care of freeing the slab name in case
of slab allocation failure then this would be better off jumping to
out_free_request_sock_slab.

quoted

        }

        mutex_lock(&proto_list_mutex);
--
2.27.0

-- 
Best regards, Tonghao