[PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[Maxime Chevallier]

There's a consistent pattern where the .cleanup_data() callback is
called when .prepare_data() fails, when it should really be called to
clean after a successfull .prepare_data() as per the documentation.

Rewrite the error-handling paths to make sure we don't cleanup
un-prepared data.

Fixes: 728480f12442 ("ethtool: default handlers for GET requests")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/netlink.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index a163d40c6431..977beeaaa2f9 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -500,7 +500,7 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
 		netdev_unlock_ops(req_info->dev);
 	rtnl_unlock();
 	if (ret < 0)
-		goto err_cleanup;
+		goto err_dev;
 	ret = ops->reply_size(req_info, reply_data);
 	if (ret < 0)
 		goto err_cleanup;
@@ -560,7 +560,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
 	netdev_unlock_ops(dev);
 	rtnl_unlock();
 	if (ret < 0)
-		goto out;
+		goto out_cancel;
 	ret = ethnl_fill_reply_header(skb, dev, ctx->ops->hdr_attr);
 	if (ret < 0)
 		goto out;
@@ -569,6 +569,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
 out:
 	if (ctx->ops->cleanup_data)
 		ctx->ops->cleanup_data(ctx->reply_data);
+out_cancel:
 	ctx->reply_data->dev = NULL;
 	if (ret < 0)
 		genlmsg_cancel(skb, ehdr);
@@ -793,7 +794,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 	ethnl_init_reply_data(reply_data, ops, dev);
 	ret = ops->prepare_data(req_info, reply_data, &info);
 	if (ret < 0)
-		goto err_cleanup;
+		goto err_rep;
 	ret = ops->reply_size(req_info, reply_data);
 	if (ret < 0)
 		goto err_cleanup;
@@ -828,6 +829,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 err_cleanup:
 	if (ops->cleanup_data)
 		ops->cleanup_data(reply_data);
+err_rep:
 	kfree(reply_data);
 	kfree(req_info);
 	return;
-- 
2.49.0

Re: [PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[Simon Horman]

On Thu, Apr 03, 2025 at 03:24:46PM +0200, Maxime Chevallier wrote:
> There's a consistent pattern where the .cleanup_data() callback is
> called when .prepare_data() fails, when it should really be called to
> clean after a successfull .prepare_data() as per the documentation.

Nit, if you have to respin for some other reason: successful

> 
> Rewrite the error-handling paths to make sure we don't cleanup
> un-prepared data.
> 
> Fixes: 728480f12442 ("ethtool: default handlers for GET requests")
> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

I agree this makes sense and addresses all instances
of this problem in this file.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[Jakub Kicinski]

On Thu,  3 Apr 2025 15:24:46 +0200 Maxime Chevallier wrote:
> There's a consistent pattern where the .cleanup_data() callback is
> called when .prepare_data() fails, when it should really be called to
> clean after a successfull .prepare_data() as per the documentation.
> 
> Rewrite the error-handling paths to make sure we don't cleanup
> un-prepared data.

Code looks good. I have a question about the oldest instance of 
the problem tho. The callbacks Michal added seem to be "idempotent".
As you say the code doesn't implement the documented model, but
I think until eeprom (?) was added the prepare callbacks could
have only failed on memory allocation, and all the cleanup did
was kfree(). So since kfree(NULL) is fine - nothing would have
crashed..

Could you repost with the Fixes tag and an explanation of where
the first instance of this causing a potential real crash was added?
-- 
pw-bot: cr

Re: [PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[Maxime Chevallier]

Hi Jakub,

On Fri, 4 Apr 2025 07:47:44 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu,  3 Apr 2025 15:24:46 +0200 Maxime Chevallier wrote:
> > There's a consistent pattern where the .cleanup_data() callback is
> > called when .prepare_data() fails, when it should really be called to
> > clean after a successfull .prepare_data() as per the documentation.
> > 
> > Rewrite the error-handling paths to make sure we don't cleanup
> > un-prepared data.  
> 
> Code looks good. I have a question about the oldest instance of 
> the problem tho. The callbacks Michal added seem to be "idempotent".
> As you say the code doesn't implement the documented model, but
> I think until eeprom (?) was added the prepare callbacks could
> have only failed on memory allocation, and all the cleanup did
> was kfree(). So since kfree(NULL) is fine - nothing would have
> crashed..
> 
> Could you repost with the Fixes tag and an explanation of where
> the first instance of this causing a potential real crash was added?

TBH I didn't even see a crash, I just stumbled upon that when working
on the phy-dump stuff. I was actually surprised that I could trace it
back so far, surely things would've blown-up somewhere in the past 6
years...

I'll look at the chronology and see what was the first point in time
where a crash could've realistically gone wrong then.

Thanks

Maxime

Re: [PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Thu, Apr 03, 2025 at 03:24:46PM +0200, Maxime Chevallier wrote:
> There's a consistent pattern where the .cleanup_data() callback is
> called when .prepare_data() fails, when it should really be called to
> clean after a successfull .prepare_data() as per the documentation.

Agreed. The rationale is that only ->prepare_data() callback knows
what exactly failed and what needs to be reverted. And it makes much
more sense for it to do the necessary cleanup than to provide enough
information for it to be done elsewhere. Except, of course, the simple
cases where ->cleanup() is just a bunch of kfree() calls with zeroing
those pointers so that it can be called repeatedly.

> 
> Rewrite the error-handling paths to make sure we don't cleanup
> un-prepared data.
> 
> Fixes: 728480f12442 ("ethtool: default handlers for GET requests")
> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]