[PATCH net-next] net: ethtool: phy: Clear the netdev context pointer for DUMP requests

[PATCH net-next] net: ethtool: phy: Clear the netdev context pointer for DUMP requests

[Maxime Chevallier]

The context info allows continuing DUMP requests, shall they fill the
netlink buffer. When performing unfiltered dump request, clear the dev
pointer in the context at the end of the dump, avoiding the release of
the netdevice. In the case of a filtered DUMP, the refcount for the
netdev was held when parsing the header, and is released in the .done()
callback.

Reported-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/000000000000d3bf150621d361a7@google.com/
Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/phy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index 560dd039c662..99d2a8b6144c 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -301,6 +301,11 @@ int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 			ctx->phy_index = 0;
 		}
+
+		/* Clear the context netdev pointer so avoid a netdev_put from
+		 * the .done() callback
+		 */
+		ctx->phy_req_info->base.dev = NULL;
 	}
 	rtnl_unlock();
 
-- 
2.46.0

Re: [PATCH net-next] net: ethtool: phy: Clear the netdev context pointer for DUMP requests

[Jakub Kicinski]

On Wed, 11 Sep 2024 15:46:21 +0200 Maxime Chevallier wrote:
> +		/* Clear the context netdev pointer so avoid a netdev_put from
> +		 * the .done() callback
> +		 */
> +		ctx->phy_req_info->base.dev = NULL;

Why do we assign to req_base.dev in the first place?
req is for the parsed request in my mind, and I don't
see anything in the PHY dump handlers actually using dev?

Re: [PATCH net-next] net: ethtool: phy: Clear the netdev context pointer for DUMP requests

[Maxime Chevallier]

Hello Jakub,

On Thu, 12 Sep 2024 20:44:38 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 11 Sep 2024 15:46:21 +0200 Maxime Chevallier wrote:
> > +		/* Clear the context netdev pointer so avoid a netdev_put from
> > +		 * the .done() callback
> > +		 */
> > +		ctx->phy_req_info->base.dev = NULL;  
> 
> Why do we assign to req_base.dev in the first place?
> req is for the parsed request in my mind, and I don't
> see anything in the PHY dump handlers actually using dev?

After reading the code back, that's true. It's just a leftover from
when I hadn't considered that dumps could be interrupted/resumed.

Let me clean that up then.

Thanks for the review Jakub,

Maxime