On Mon, Mar 8, 2021 at 7:15 PM <xiangxia.m....@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m....@gmail.com>
>
> Introduce a new function twsk_prot_init, inspired by
> req_prot_init, to simplify the "proto_register" function.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m....@gmail.com>
> ---
> 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.
> 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
>
