netdev - Re: [PATCH 1/2] net/iucv: Improve unlocking in iucv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d2e5c263-c0aa-4297-b446-f013af7eb80f@web.de>
Date: Tue, 2 Jan 2024 11:31:35 +0100
From: Markus Elfring <Markus.Elfring@....de>
To: Alexandra Winter <wintera@...ux.ibm.com>, Suman Ghosh
 <sumang@...vell.com>, "linux-s390@...r.kernel.org"
 <linux-s390@...r.kernel.org>, "netdev@...r.kernel.org"
 <netdev@...r.kernel.org>,
 "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Wenjia Zhang <wenjia@...ux.ibm.com>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] net/iucv: Improve unlocking in iucv_enable()

> I share Suman's concern that jumping backwards goto is confusing.
> But I think the Coccinelle finding of freeing a null-pointer should be addressed (see patch 2/2)
> Thank you Markus for reporting it.
>
> The allocation does require holding the cpus_read_lock.

How does this information fit to your following suggestion to adjust the lock scope?

> For some reason Markus wants to reduce the number of cpus_read_unlock() calls (why?),

One cpus_read_unlock() call is required here.
Would you like to benefit more from a smaller executable code size?

> so what about something like this for both issues:
>
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 0ed6e34d6edd..1030403b826b 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -542,24 +542,22 @@ static int iucv_enable(void)
>         size_t alloc_size;
>         int cpu, rc;
>
> -       cpus_read_lock();
> -       rc = -ENOMEM;
>         alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
>         iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
>         if (!iucv_path_table)
> -               goto out;
> +               return -ENOMEM;
>         /* Declare per cpu buffers. */
> -       rc = -EIO;
> +       cpus_read_lock();
>         for_each_online_cpu(cpu)
>                 smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
> -       if (cpumask_empty(&iucv_buffer_cpumask))
> +       if (cpumask_empty(&iucv_buffer_cpumask)) {
>                 /* No cpu could declare an iucv buffer. */
> -               goto out;
> -       cpus_read_unlock();
> -       return 0;
> -out:
> -       kfree(iucv_path_table);
> -       iucv_path_table = NULL;
> +               kfree(iucv_path_table);
> +               iucv_path_table = NULL;
> +               rc = -EIO;
> +       } else {
> +               rc = 0;
> +       }
>         cpus_read_unlock();
>         return rc;
>  }

I suggest to reconsider patch squashing a bit more.

Regards,
Markus