netdev - Re: [PATCH v2 net-next] sfc: Use netdev refcount tracking in struct efx_async_filter

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: <CAA-VZP=NxTVR4wyTzRtm4tX7v_5RrFLvN4k5kX8oDEMR=SsioA@mail.gmail.com>
Date: Wed, 18 Dec 2024 13:54:50 -0800
From: YiFei Zhu <zhuyifei@...gle.com>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: Martin Habets <habetsm.xilinx@...il.com>, netdev@...r.kernel.org, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	linux-net-drivers@....com, Willem de Bruijn <willemb@...gle.com>, 
	Mina Almasry <almasrymina@...gle.com>
Subject: Re: [PATCH v2 net-next] sfc: Use netdev refcount tracking in struct efx_async_filter_insertion

On Wed, Dec 18, 2024 at 2:09 AM Edward Cree <ecree.xilinx@...il.com> wrote:
>
> On 17/12/2024 22:47, YiFei Zhu wrote:
> > I was debugging some netdev refcount issues in OpenOnload, and one
> > of the places I was looking at was in the sfc driver. Only
> > struct efx_async_filter_insertion was not using netdev refcount tracker,
> > so add it here. GFP_ATOMIC because this code path is called by
> > ndo_rx_flow_steer which holds RCU.
> >
> > This patch should be a no-op if !CONFIG_NET_DEV_REFCNT_TRACKER
> >
> > Signed-off-by: YiFei Zhu <zhuyifei@...gle.com>
> > ---
> > v1 -> v2:
> > - Documented the added field of @net_dev_tracker in the struct
>
> Please do not post new versions of a patch within 24 hours, see
>  https://docs.kernel.org/process/maintainer-netdev.html#resending-after-review

My apologies. I noticed the missing docstring while backporting the
refcount tracking in tc_encap_actions.c to OpenOnload, so I thought
I'd send a revision to avoid inconsistencies, and to avoid wasting
reviewer time to point out something that I've already noticed. Was
not aware of the 24h rule, sorry.

> > @@ -989,7 +989,7 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
> >       }
> >
> >       /* Queue the request */
> > -     dev_hold(req->net_dev = net_dev);
> > +     netdev_hold(req->net_dev = net_dev, &req->net_dev_tracker, GFP_ATOMIC);
>
> This line becomes sufficiently complex that the assignment to
>  req->net_dev should be separated out into its own statement.
> (And the same thing in the siena equivalent.)

Will fix it in v3.

> Other than that the direction of this patch looks okay.  Have you
>  tested it with the kconfig enabled?

Only a compile test. It was a blanket change from dev_hold ->
netdev_hold in OpenOnload to wherever it made obvious sense. It was
only after, that I found that it was the driver for solarflare NIC and
was told it was a copy of the in-kernel driver. Since I don't have a
solarflare NIC on hand, the code path is not exercised.

YiFei Zhu