| Message ID | 20240227092411.2315725-3-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bbcf91053bb622c4c26a9bfc998d3b0c59227f10 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | inet: implement lockless RTM_GETNETCONF ops | expand |
Tue, Feb 27, 2024 at 10:24:10AM CET, edumazet@google.com wrote: >"ip -4 netconf show dev XXXX" no longer acquires RTNL. I was under impression that you refer to the current code, confused me a bit :/ > >Return -ENODEV instead of -EINVAL if no netdev or idev can be found. > >Signed-off-by: Eric Dumazet <edumazet@google.com> >--- > net/ipv4/devinet.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > >diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c >index ca75d0fff1d1ebd8c199fb74a6f0e2f51160635c..f045a34e90b974b17512a30c3b719bdfc3cba153 100644 >--- a/net/ipv4/devinet.c >+++ b/net/ipv4/devinet.c >@@ -2205,21 +2205,20 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, > struct netlink_ext_ack *extack) > { > struct net *net = sock_net(in_skb->sk); >- struct nlattr *tb[NETCONFA_MAX+1]; >+ struct nlattr *tb[NETCONFA_MAX + 1]; >+ const struct ipv4_devconf *devconf; >+ struct in_device *in_dev = NULL; >+ struct net_device *dev = NULL; > struct sk_buff *skb; >- struct ipv4_devconf *devconf; >- struct in_device *in_dev; >- struct net_device *dev; > int ifindex; > int err; > > err = inet_netconf_valid_get_req(in_skb, nlh, tb, extack); > if (err) >- goto errout; >+ return err; > >- err = -EINVAL; > if (!tb[NETCONFA_IFINDEX]) >- goto errout; >+ return -EINVAL; > > ifindex = nla_get_s32(tb[NETCONFA_IFINDEX]); > switch (ifindex) { >@@ -2230,10 +2229,10 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, > devconf = net->ipv4.devconf_dflt; > break; > default: >- dev = __dev_get_by_index(net, ifindex); >- if (!dev) >- goto errout; >- in_dev = __in_dev_get_rtnl(dev); >+ err = -ENODEV; >+ dev = dev_get_by_index(net, ifindex); Comment says: /* Deprecated for new users, call netdev_get_by_index() instead */ struct net_device *dev_get_by_index(struct net *net, int ifindex) Perhaps better to use: netdev_get_by_index() and netdev_put()? >+ if (dev) >+ in_dev = in_dev_get(dev); The original flow: err = -ENODEV; dev = dev_get_by_index(net, ifindex); if (!dev) goto errout; in_dev = in_dev_get(dev); if (!in_dev) goto errout; Reads a bit nicer to me. Not sure why you changed it. Yeah, it's a nit. > if (!in_dev) > goto errout; > devconf = &in_dev->cnf; >@@ -2257,6 +2256,9 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, > } > err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); > errout: >+ if (in_dev) >+ in_dev_put(in_dev); >+ dev_put(dev); > return err; > } > >@@ -2826,5 +2828,6 @@ void __init devinet_init(void) > rtnl_register(PF_INET, RTM_DELADDR, inet_rtm_deladdr, NULL, 0); > rtnl_register(PF_INET, RTM_GETADDR, NULL, inet_dump_ifaddr, 0); > rtnl_register(PF_INET, RTM_GETNETCONF, inet_netconf_get_devconf, >- inet_netconf_dump_devconf, 0); >+ inet_netconf_dump_devconf, >+ RTNL_FLAG_DOIT_UNLOCKED); > } >-- >2.44.0.rc1.240.g4c46232300-goog > >
On Tue, Feb 27, 2024 at 1:59 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Tue, Feb 27, 2024 at 10:24:10AM CET, edumazet@google.com wrote: > >"ip -4 netconf show dev XXXX" no longer acquires RTNL. > > I was under impression that you refer to the current code, confused me a > bit :/ > > > > > >Return -ENODEV instead of -EINVAL if no netdev or idev can be found. > > > >Signed-off-by: Eric Dumazet <edumazet@google.com> > >--- > > net/ipv4/devinet.c | 27 +++++++++++++++------------ > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > >diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > >index ca75d0fff1d1ebd8c199fb74a6f0e2f51160635c..f045a34e90b974b17512a30c3b719bdfc3cba153 100644 > >--- a/net/ipv4/devinet.c > >+++ b/net/ipv4/devinet.c > >@@ -2205,21 +2205,20 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, > > struct netlink_ext_ack *extack) > > { > > struct net *net = sock_net(in_skb->sk); > >- struct nlattr *tb[NETCONFA_MAX+1]; > >+ struct nlattr *tb[NETCONFA_MAX + 1]; > >+ const struct ipv4_devconf *devconf; > >+ struct in_device *in_dev = NULL; > >+ struct net_device *dev = NULL; > > struct sk_buff *skb; > >- struct ipv4_devconf *devconf; > >- struct in_device *in_dev; > >- struct net_device *dev; > > int ifindex; > > int err; > > > > err = inet_netconf_valid_get_req(in_skb, nlh, tb, extack); > > if (err) > >- goto errout; > >+ return err; > > > >- err = -EINVAL; > > if (!tb[NETCONFA_IFINDEX]) > >- goto errout; > >+ return -EINVAL; > > > > ifindex = nla_get_s32(tb[NETCONFA_IFINDEX]); > > switch (ifindex) { > >@@ -2230,10 +2229,10 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, > > devconf = net->ipv4.devconf_dflt; > > break; > > default: > >- dev = __dev_get_by_index(net, ifindex); > >- if (!dev) > >- goto errout; > >- in_dev = __in_dev_get_rtnl(dev); > >+ err = -ENODEV; > >+ dev = dev_get_by_index(net, ifindex); > > Comment says: > /* Deprecated for new users, call netdev_get_by_index() instead */ > struct net_device *dev_get_by_index(struct net *net, int ifindex) Only for long-standing allocations, where we are not sure if a leak could happen or not. We do not bother allocating a tracker otherwise. Look at inet6_netconf_get_devconf() : We left there dev_get_by_index() and dev_put(). I think I am aware of the tracking facility, I implemented it... > > Perhaps better to use: > netdev_get_by_index() and netdev_put()? > > > >+ if (dev) > >+ in_dev = in_dev_get(dev); > > The original flow: > err = -ENODEV; > dev = dev_get_by_index(net, ifindex); > if (!dev) > goto errout; > in_dev = in_dev_get(dev); > if (!in_dev) > goto errout; A single goto looks nicer to me. > Reads a bit nicer to me. Not sure why you changed it. Yeah, it's a nit. >
Tue, Feb 27, 2024 at 02:09:49PM CET, edumazet@google.com wrote: >On Tue, Feb 27, 2024 at 1:59 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Tue, Feb 27, 2024 at 10:24:10AM CET, edumazet@google.com wrote: >> >"ip -4 netconf show dev XXXX" no longer acquires RTNL. >> >> I was under impression that you refer to the current code, confused me a >> bit :/ >> >> >> > >> >Return -ENODEV instead of -EINVAL if no netdev or idev can be found. >> > >> >Signed-off-by: Eric Dumazet <edumazet@google.com> >> >--- >> > net/ipv4/devinet.c | 27 +++++++++++++++------------ >> > 1 file changed, 15 insertions(+), 12 deletions(-) >> > >> >diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c >> >index ca75d0fff1d1ebd8c199fb74a6f0e2f51160635c..f045a34e90b974b17512a30c3b719bdfc3cba153 100644 >> >--- a/net/ipv4/devinet.c >> >+++ b/net/ipv4/devinet.c >> >@@ -2205,21 +2205,20 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, >> > struct netlink_ext_ack *extack) >> > { >> > struct net *net = sock_net(in_skb->sk); >> >- struct nlattr *tb[NETCONFA_MAX+1]; >> >+ struct nlattr *tb[NETCONFA_MAX + 1]; >> >+ const struct ipv4_devconf *devconf; >> >+ struct in_device *in_dev = NULL; >> >+ struct net_device *dev = NULL; >> > struct sk_buff *skb; >> >- struct ipv4_devconf *devconf; >> >- struct in_device *in_dev; >> >- struct net_device *dev; >> > int ifindex; >> > int err; >> > >> > err = inet_netconf_valid_get_req(in_skb, nlh, tb, extack); >> > if (err) >> >- goto errout; >> >+ return err; >> > >> >- err = -EINVAL; >> > if (!tb[NETCONFA_IFINDEX]) >> >- goto errout; >> >+ return -EINVAL; >> > >> > ifindex = nla_get_s32(tb[NETCONFA_IFINDEX]); >> > switch (ifindex) { >> >@@ -2230,10 +2229,10 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, >> > devconf = net->ipv4.devconf_dflt; >> > break; >> > default: >> >- dev = __dev_get_by_index(net, ifindex); >> >- if (!dev) >> >- goto errout; >> >- in_dev = __in_dev_get_rtnl(dev); >> >+ err = -ENODEV; >> >+ dev = dev_get_by_index(net, ifindex); >> >> Comment says: >> /* Deprecated for new users, call netdev_get_by_index() instead */ >> struct net_device *dev_get_by_index(struct net *net, int ifindex) > >Only for long-standing allocations, where we are not sure if a leak >could happen or not. >We do not bother allocating a tracker otherwise. Makes sense. Would it make sense to fix the "deprecated" comment then to also reflect this usecase? >Look at inet6_netconf_get_devconf() : >We left there dev_get_by_index() and dev_put(). > >I think I am aware of the tracking facility, I implemented it... Yeah, I was just refering to the comment. > > >> >> Perhaps better to use: >> netdev_get_by_index() and netdev_put()? >> >> >> >+ if (dev) >> >+ in_dev = in_dev_get(dev); >> >> The original flow: >> err = -ENODEV; >> dev = dev_get_by_index(net, ifindex); >> if (!dev) >> goto errout; >> in_dev = in_dev_get(dev); >> if (!in_dev) >> goto errout; > >A single goto looks nicer to me. :) Reviewed-by: Jiri Pirko <jiri@nvidia.com> > >> Reads a bit nicer to me. Not sure why you changed it. Yeah, it's a nit. >>
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index ca75d0fff1d1ebd8c199fb74a6f0e2f51160635c..f045a34e90b974b17512a30c3b719bdfc3cba153 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -2205,21 +2205,20 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, struct netlink_ext_ack *extack) { struct net *net = sock_net(in_skb->sk); - struct nlattr *tb[NETCONFA_MAX+1]; + struct nlattr *tb[NETCONFA_MAX + 1]; + const struct ipv4_devconf *devconf; + struct in_device *in_dev = NULL; + struct net_device *dev = NULL; struct sk_buff *skb; - struct ipv4_devconf *devconf; - struct in_device *in_dev; - struct net_device *dev; int ifindex; int err; err = inet_netconf_valid_get_req(in_skb, nlh, tb, extack); if (err) - goto errout; + return err; - err = -EINVAL; if (!tb[NETCONFA_IFINDEX]) - goto errout; + return -EINVAL; ifindex = nla_get_s32(tb[NETCONFA_IFINDEX]); switch (ifindex) { @@ -2230,10 +2229,10 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, devconf = net->ipv4.devconf_dflt; break; default: - dev = __dev_get_by_index(net, ifindex); - if (!dev) - goto errout; - in_dev = __in_dev_get_rtnl(dev); + err = -ENODEV; + dev = dev_get_by_index(net, ifindex); + if (dev) + in_dev = in_dev_get(dev); if (!in_dev) goto errout; devconf = &in_dev->cnf; @@ -2257,6 +2256,9 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb, } err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); errout: + if (in_dev) + in_dev_put(in_dev); + dev_put(dev); return err; } @@ -2826,5 +2828,6 @@ void __init devinet_init(void) rtnl_register(PF_INET, RTM_DELADDR, inet_rtm_deladdr, NULL, 0); rtnl_register(PF_INET, RTM_GETADDR, NULL, inet_dump_ifaddr, 0); rtnl_register(PF_INET, RTM_GETNETCONF, inet_netconf_get_devconf, - inet_netconf_dump_devconf, 0); + inet_netconf_dump_devconf, + RTNL_FLAG_DOIT_UNLOCKED); }
"ip -4 netconf show dev XXXX" no longer acquires RTNL. Return -ENODEV instead of -EINVAL if no netdev or idev can be found. Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/ipv4/devinet.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)