| Message ID | 20250403132448.405266-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ethtool: Don't call .cleanup_data when prepare_data fails | expand |
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>
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?
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
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
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;
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(-)