[PATCH] net: sock: simplify tw proto registration

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;
+}
+
 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;
 	}
 
 	mutex_lock(&proto_list_mutex);

-- 
2.27.0

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

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.

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.

        }

        mutex_lock(&proto_list_mutex);
--
2.27.0

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

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

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.

Otherwise the patch set itself looks good to me.

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

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