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

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

On Tue, Mar 9, 2021 at 5:48 PM Tonghao Zhang [off-list ref] wrote:

quoted

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

quoted

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().

I see. Okay so the expectation is that tw_prot_cleanup will take care
of a partially initialized timewait_sock_ops.

With that being the case the one change I would ask you to make would
be to look at moving the function up so it is just below
tw_prot_cleanup so it is obvious that the two are meant to be paired
rather than placing it after req_prot_init.

Thanks, will be changed in v2
and change the new function name from twsk_prot_init to tw_prot_init
(tw_prot_cleanup).

Otherwise the patch set itself looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

-- 
Best regards, Tonghao