netdev - Re: [PATCH net] net: fix the missing unlock for detached devices

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izMnK0C0sQpK+5NoL-ETNjJ+6BUhx_Bgq9drViUaic+W1A@mail.gmail.com>
Date: Fri, 18 Apr 2025 04:51:26 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, 
	jdamato@...tly.com, sdf@...ichev.me, ap420073@...il.com
Subject: Re: [PATCH net] net: fix the missing unlock for detached devices

On Thu, Apr 17, 2025 at 6:53 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> The combined condition was left as is when we converted
> from __dev_get_by_index() to netdev_get_by_index_lock().
> There was no need to undo anything with the former, for
> the latter we need an unlock.
>
> Fixes: 1d22d3060b9b ("net: drop rtnl_lock for queue_mgmt operations")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Reviewed-by: Mina Almasry <almasrymina@...gle.com>

> ---
> CC: jdamato@...tly.com
> CC: almasrymina@...gle.com
> CC: sdf@...ichev.me
> CC: ap420073@...il.com
> ---
>  net/core/netdev-genl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 5d7af50fe702..230743bdbb14 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -861,14 +861,17 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
>
>         mutex_lock(&priv->lock);
>
> +       err = 0;

nit: AFAICT, this setting to 0 is redundant. err is 0 initialized and
I don't see a code path that sets err but doesn't goto.

>         netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
> -       if (!netdev || !netif_device_present(netdev)) {
> +       if (!netdev) {
>                 err = -ENODEV;
>                 goto err_unlock_sock;
>         }
> -
> -       if (!netdev_need_ops_lock(netdev)) {
> +       if (!netif_device_present(netdev))
> +               err = -ENODEV;
> +       else if (!netdev_need_ops_lock(netdev))
>                 err = -EOPNOTSUPP;
> +       if (err) {
>                 NL_SET_BAD_ATTR(info->extack,
>                                 info->attrs[NETDEV_A_DEV_IFINDEX]);
>                 goto err_unlock;
> --
> 2.49.0
>


-- 
Thanks,
Mina