Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

net: ethtool: fix leaking netdev ref if ethnl_default_parse() failed

Ido spotted that I made a mistake in commit under Fixes,
ethnl_default_parse() may acquire a dev reference even when it returns
an error. This may have been driven by the code structure in dumps
(which unconditionally release dev before handling errors), but it's
too much of a trap. Functions should undo what they did before returning
an error, rather than expecting caller to clean up.

Rather than fixing ethnl_default_set_doit() directly make
ethnl_default_parse() clean up errors.

Reported-by: Ido Schimmel <idosch@idosch.org>
Link: https://lore.kernel.org/aGEPszpq9eojNF4Y@shredder
Fixes: 963781bdfe20 ("net: ethtool: call .parse_request for SET handlers")
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Link: https://patch.msgid.link/20250630154053.1074664-1-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

+12 -6
+12 -6
net/ethtool/netlink.c
··· 455 455 if (request_ops->parse_request) { 456 456 ret = request_ops->parse_request(req_info, tb, info->extack); 457 457 if (ret < 0) 458 - return ret; 458 + goto err_dev; 459 459 } 460 460 461 461 return 0; 462 + 463 + err_dev: 464 + netdev_put(req_info->dev, &req_info->dev_tracker); 465 + req_info->dev = NULL; 466 + return ret; 462 467 } 463 468 464 469 /** ··· 513 508 514 509 ret = ethnl_default_parse(req_info, info, ops, !ops->allow_nodev_do); 515 510 if (ret < 0) 516 - goto err_dev; 511 + goto err_free; 517 512 ethnl_init_reply_data(reply_data, ops, req_info->dev); 518 513 519 514 rtnl_lock(); ··· 559 554 ops->cleanup_data(reply_data); 560 555 err_dev: 561 556 netdev_put(req_info->dev, &req_info->dev_tracker); 557 + err_free: 562 558 kfree(reply_data); 563 559 kfree(req_info); 564 560 return ret; ··· 662 656 } 663 657 664 658 ret = ethnl_default_parse(req_info, &info->info, ops, false); 659 + if (ret < 0) 660 + goto free_reply_data; 665 661 if (req_info->dev) { 666 662 /* We ignore device specification in dump requests but as the 667 663 * same parser as for non-dump (doit) requests is used, it ··· 672 664 netdev_put(req_info->dev, &req_info->dev_tracker); 673 665 req_info->dev = NULL; 674 666 } 675 - if (ret < 0) 676 - goto free_reply_data; 677 667 678 668 ctx->ops = ops; 679 669 ctx->req_info = req_info; ··· 720 714 * the dev's ifindex, .dumpit() will grab and release the netdev itself. 721 715 */ 722 716 ret = ethnl_default_parse(req_info, &info->info, ops, false); 717 + if (ret < 0) 718 + goto free_reply_data; 723 719 if (req_info->dev) { 724 720 phy_ctx->ifindex = req_info->dev->ifindex; 725 721 netdev_put(req_info->dev, &req_info->dev_tracker); 726 722 req_info->dev = NULL; 727 723 } 728 - if (ret < 0) 729 - goto free_reply_data; 730 724 731 725 ctx->ops = ops; 732 726 ctx->req_info = req_info;