Re: [PATCH] net: sock: simplify tw proto registration
From: Alexander Duyck <hidden>
Date: 2021-03-09 17:40:39
On Mon, Mar 8, 2021 at 7:15 PM [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
}
mutex_lock(&proto_list_mutex);
--
2.27.0