| Message ID | 20200322201551.E11BAE0FD3@unicorn.suse.cz |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ethtool: fix reference leak in some *_SET handlers | expand |
On Sun, Mar 22, 2020 at 09:15:51PM +0100, Michal Kubecek wrote: > Andrew noticed that some handlers for *_SET commands leak a netdev > reference if required ethtool_ops callbacks do not exist. A simple > reproducer would be e.g. > > ip link add veth1 type veth peer name veth2 > ethtool -s veth1 wol g > ip link del veth1 > > Make sure dev_put() is called when ethtool_ops check fails. > > Reported-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 3/22/2020 1:15 PM, Michal Kubecek wrote: > Andrew noticed that some handlers for *_SET commands leak a netdev > reference if required ethtool_ops callbacks do not exist. A simple > reproducer would be e.g. > > ip link add veth1 type veth peer name veth2 > ethtool -s veth1 wol g > ip link del veth1 > > Make sure dev_put() is called when ethtool_ops check fails. > > Reported-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> Would not you want a Fixes: tag for this change? Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Sun, 22 Mar 2020 21:15:51 +0100 (CET) Michal Kubecek wrote: > Andrew noticed that some handlers for *_SET commands leak a netdev > reference if required ethtool_ops callbacks do not exist. A simple > reproducer would be e.g. > > ip link add veth1 type veth peer name veth2 > ethtool -s veth1 wol g > ip link del veth1 > > Make sure dev_put() is called when ethtool_ops check fails. Fixes: e54d04e3afea ("ethtool: set message mask with DEBUG_SET request") Fixes: a53f3d41e4d3 ("ethtool: set link settings with LINKINFO_SET request") Fixes: bfbcfe2032e7 ("ethtool: set link modes related data with LINKMODES_SET request") Fixes: 8d425b19b305 ("ethtool: set wake-on-lan settings with WOL_SET request") > Reported-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
On Sun, Mar 22, 2020 at 01:43:56PM -0700, Jakub Kicinski wrote: > On Sun, 22 Mar 2020 21:15:51 +0100 (CET) Michal Kubecek wrote: > > Andrew noticed that some handlers for *_SET commands leak a netdev > > reference if required ethtool_ops callbacks do not exist. A simple > > reproducer would be e.g. > > > > ip link add veth1 type veth peer name veth2 > > ethtool -s veth1 wol g > > ip link del veth1 > > > > Make sure dev_put() is called when ethtool_ops check fails. > > Fixes: e54d04e3afea ("ethtool: set message mask with DEBUG_SET request") > Fixes: a53f3d41e4d3 ("ethtool: set link settings with LINKINFO_SET request") > Fixes: bfbcfe2032e7 ("ethtool: set link modes related data with LINKMODES_SET request") > Fixes: 8d425b19b305 ("ethtool: set wake-on-lan settings with WOL_SET request") Yes, thank you, I forgot about Fixes tags. Should I resubmit or will patchworks pick the tags from your reply? Michal > > Reported-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > > Reviewed-by: Jakub Kicinski <kuba@kernel.org>
On Sun, 22 Mar 2020 21:51:09 +0100 Michal Kubecek wrote: > On Sun, Mar 22, 2020 at 01:43:56PM -0700, Jakub Kicinski wrote: > > On Sun, 22 Mar 2020 21:15:51 +0100 (CET) Michal Kubecek wrote: > > > Andrew noticed that some handlers for *_SET commands leak a netdev > > > reference if required ethtool_ops callbacks do not exist. A simple > > > reproducer would be e.g. > > > > > > ip link add veth1 type veth peer name veth2 > > > ethtool -s veth1 wol g > > > ip link del veth1 > > > > > > Make sure dev_put() is called when ethtool_ops check fails. > > > > Fixes: e54d04e3afea ("ethtool: set message mask with DEBUG_SET request") > > Fixes: a53f3d41e4d3 ("ethtool: set link settings with LINKINFO_SET request") > > Fixes: bfbcfe2032e7 ("ethtool: set link modes related data with LINKMODES_SET request") > > Fixes: 8d425b19b305 ("ethtool: set wake-on-lan settings with WOL_SET request") > > Yes, thank you, I forgot about Fixes tags. > > Should I resubmit or will patchworks pick the tags from your reply? Patchwork sees them, I think, but I don't think it adds them to the patch as downloaded by git-pw. Probably easiest to repost. > > > Reported-by: Andrew Lunn <andrew@lunn.ch> > > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > > > > Reviewed-by: Jakub Kicinski <kuba@kernel.org>
On Sun, Mar 22, 2020 at 02:06:23PM -0700, Jakub Kicinski wrote: > On Sun, 22 Mar 2020 21:51:09 +0100 Michal Kubecek wrote: > > On Sun, Mar 22, 2020 at 01:43:56PM -0700, Jakub Kicinski wrote: > > > On Sun, 22 Mar 2020 21:15:51 +0100 (CET) Michal Kubecek wrote: > > > > Andrew noticed that some handlers for *_SET commands leak a netdev > > > > reference if required ethtool_ops callbacks do not exist. A simple > > > > reproducer would be e.g. > > > > > > > > ip link add veth1 type veth peer name veth2 > > > > ethtool -s veth1 wol g > > > > ip link del veth1 > > > > > > > > Make sure dev_put() is called when ethtool_ops check fails. > > > > > > Fixes: e54d04e3afea ("ethtool: set message mask with DEBUG_SET request") > > > Fixes: a53f3d41e4d3 ("ethtool: set link settings with LINKINFO_SET request") > > > Fixes: bfbcfe2032e7 ("ethtool: set link modes related data with LINKMODES_SET request") > > > Fixes: 8d425b19b305 ("ethtool: set wake-on-lan settings with WOL_SET request") > > > > Yes, thank you, I forgot about Fixes tags. > > > > Should I resubmit or will patchworks pick the tags from your reply? > > Patchwork sees them, I think, but I don't think it adds them to the > patch as downloaded by git-pw. Probably easiest to repost. Sent v2 with Fixes and Reviewed-by tags. Michal > > > > Reported-by: Andrew Lunn <andrew@lunn.ch> > > > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > > > > > > Reviewed-by: Jakub Kicinski <kuba@kernel.org> >
diff --git a/net/ethtool/debug.c b/net/ethtool/debug.c index aaef4843e6ba..92599ad7b3c2 100644 --- a/net/ethtool/debug.c +++ b/net/ethtool/debug.c @@ -107,8 +107,9 @@ int ethnl_set_debug(struct sk_buff *skb, struct genl_info *info) if (ret < 0) return ret; dev = req_info.dev; + ret = -EOPNOTSUPP; if (!dev->ethtool_ops->get_msglevel || !dev->ethtool_ops->set_msglevel) - return -EOPNOTSUPP; + goto out_dev; rtnl_lock(); ret = ethnl_ops_begin(dev); @@ -129,6 +130,7 @@ int ethnl_set_debug(struct sk_buff *skb, struct genl_info *info) ethnl_ops_complete(dev); out_rtnl: rtnl_unlock(); +out_dev: dev_put(dev); return ret; } diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c index 5d16cb4e8693..6e9e0b590bb5 100644 --- a/net/ethtool/linkinfo.c +++ b/net/ethtool/linkinfo.c @@ -126,9 +126,10 @@ int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info) if (ret < 0) return ret; dev = req_info.dev; + ret = -EOPNOTSUPP; if (!dev->ethtool_ops->get_link_ksettings || !dev->ethtool_ops->set_link_ksettings) - return -EOPNOTSUPP; + goto out_dev; rtnl_lock(); ret = ethnl_ops_begin(dev); @@ -162,6 +163,7 @@ int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info) ethnl_ops_complete(dev); out_rtnl: rtnl_unlock(); +out_dev: dev_put(dev); return ret; } diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c index 96f20be64553..18cc37be2d9c 100644 --- a/net/ethtool/linkmodes.c +++ b/net/ethtool/linkmodes.c @@ -338,9 +338,10 @@ int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info) if (ret < 0) return ret; dev = req_info.dev; + ret = -EOPNOTSUPP; if (!dev->ethtool_ops->get_link_ksettings || !dev->ethtool_ops->set_link_ksettings) - return -EOPNOTSUPP; + goto out_dev; rtnl_lock(); ret = ethnl_ops_begin(dev); @@ -370,6 +371,7 @@ int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info) ethnl_ops_complete(dev); out_rtnl: rtnl_unlock(); +out_dev: dev_put(dev); return ret; } diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c index e1b8a65b64c4..55e1ecaaf739 100644 --- a/net/ethtool/wol.c +++ b/net/ethtool/wol.c @@ -128,8 +128,9 @@ int ethnl_set_wol(struct sk_buff *skb, struct genl_info *info) if (ret < 0) return ret; dev = req_info.dev; + ret = -EOPNOTSUPP; if (!dev->ethtool_ops->get_wol || !dev->ethtool_ops->set_wol) - return -EOPNOTSUPP; + goto out_dev; rtnl_lock(); ret = ethnl_ops_begin(dev); @@ -172,6 +173,7 @@ int ethnl_set_wol(struct sk_buff *skb, struct genl_info *info) ethnl_ops_complete(dev); out_rtnl: rtnl_unlock(); +out_dev: dev_put(dev); return ret; }
Andrew noticed that some handlers for *_SET commands leak a netdev reference if required ethtool_ops callbacks do not exist. A simple reproducer would be e.g. ip link add veth1 type veth peer name veth2 ethtool -s veth1 wol g ip link del veth1 Make sure dev_put() is called when ethtool_ops check fails. Reported-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Michal Kubecek <mkubecek@suse.cz> --- net/ethtool/debug.c | 4 +++- net/ethtool/linkinfo.c | 4 +++- net/ethtool/linkmodes.c | 4 +++- net/ethtool/wol.c | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-)