netdev - Re: [PATCH net-next 2/6] net: provide pending ring configuration in net

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: <CACKFLinRxGgrgz8LUROsK0O+KTk=4a2B=mF-b2269JU+CigFPQ@mail.gmail.com>
Date: Fri, 17 Jan 2025 14:07:03 -0800
From: Michael Chan <michael.chan@...adcom.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, 
	pavan.chebbi@...adcom.com, ap420073@...il.com
Subject: Re: [PATCH net-next 2/6] net: provide pending ring configuration in net_device

On Fri, Jan 17, 2025 at 11:48 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Record the pending configuration in net_device struct.
> ethtool core duplicates the current config and the specific
> handlers (for now just ringparam) can modify it.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

> @@ -688,21 +690,35 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
>                         goto out_dev;
>         }
>
> +       dev = req_info.dev;
> +
> +       dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
> +                                  GFP_KERNEL_ACCOUNT);
> +       if (!dev->cfg_pending) {
> +               ret = -ENOMEM;
> +               goto out_tie_cfg;
> +       }
> +
>         rtnl_lock();
> -       ret = ethnl_ops_begin(req_info.dev);
> +       ret = ethnl_ops_begin(dev);
>         if (ret < 0)
>                 goto out_rtnl;
>
>         ret = ops->set(&req_info, info);
>         if (ret <= 0)
>                 goto out_ops;
> -       ethtool_notify(req_info.dev, ops->set_ntf_cmd, NULL);
> +       ethtool_notify(dev, ops->set_ntf_cmd, NULL);
>
>         ret = 0;
>  out_ops:
> -       ethnl_ops_complete(req_info.dev);
> +       ethnl_ops_complete(dev);
>  out_rtnl:
>         rtnl_unlock();
> +       if (ret >= 0) /* success! */
> +               swap(dev->cfg, dev->cfg_pending);
> +       kfree(dev->cfg_pending);
> +out_tie_cfg:
> +       dev->cfg_pending = dev->cfg;

If I understand this correctly, cfg_pending can momentarily point to
the old cfg and freed memory before pointing to the new cfg outside of
rtnl_lock.  Shouldn't it be inside the rtnl_lock?

In the bnxt patch, we now look at cfg_pending so it must always point
to the correct cfg.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)