diff mbox series

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

Message ID 20240911134623.1739633-1-maxime.chevallier@bootlin.com (mailing list archive)
State New, archived
Headers show
Series [net-next] net: ethtool: phy: Clear the netdev context pointer for DUMP requests | expand

Commit Message

Maxime Chevallier Sept. 11, 2024, 1:46 p.m. UTC
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(+)

Comments

Jakub Kicinski Sept. 13, 2024, 3:44 a.m. UTC | #1
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?
Maxime Chevallier Sept. 13, 2024, 7:14 a.m. UTC | #2
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
diff mbox series

Patch

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();