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 for Android: free password hash cracker in your pocket

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

Message-ID: <3280aeeb-bddc-ee1f-b33b-eab95a91084d@gmail.com>
Date: Wed, 18 Dec 2024 10:09:48 +0000
From: Edward Cree <ecree.xilinx@...il.com>
To: YiFei Zhu <zhuyifei@...gle.com>, Martin Habets
 <habetsm.xilinx@...il.com>, netdev@...r.kernel.org
Cc: 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 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

> @@ -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.)

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