| Message ID | 20220415165330.10497-5-florent.fourcot@wifirst.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b6177d3240a4f58fe547891010ad77a45bc1c9ab |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage | expand |
On Fri, 15 Apr 2022 18:53:30 +0200 Florent Fourcot <florent.fourcot@wifirst.fr> wrote: > A request without interface name/interface index/interface group cannot > work. We should return EINVAL > > Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr> > Signed-off-by: Brian Baboch <brian.baboch@wifirst.fr> > --- > net/core/rtnetlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 73f2cbc440c9..b943336908a7 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3457,7 +3457,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, > return rtnl_group_changelink(skb, net, > nla_get_u32(tb[IFLA_GROUP]), > ifm, extack, tb); > - return -ENODEV; > + return -EINVAL; Sometimes changing errno can be viewed as ABI change and break applications.
>> - return -ENODEV; >> + return -EINVAL; > > Sometimes changing errno can be viewed as ABI change and break applications. I agree, but I think this one is OK. __rtnl_newlink function has more than 20 return, I don't see how an application can have behavior based on this specific path. And actually, patch 1/4 is protecting almost all previous callers, this return is now only here for calls without ifindex/ifname/ifgroup, and NLM_F_CREATE not set. If you think that this change can be merged, I can add extack error at this place. EINVAL is indeed very hard to parse.
Stephen Hemminger <stephen@networkplumber.org> writes: > On Fri, 15 Apr 2022 18:53:30 +0200 > Florent Fourcot <florent.fourcot@wifirst.fr> wrote: > >> A request without interface name/interface index/interface group cannot >> work. We should return EINVAL >> >> Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr> >> Signed-off-by: Brian Baboch <brian.baboch@wifirst.fr> >> --- >> net/core/rtnetlink.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index 73f2cbc440c9..b943336908a7 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -3457,7 +3457,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, >> return rtnl_group_changelink(skb, net, >> nla_get_u32(tb[IFLA_GROUP]), >> ifm, extack, tb); >> - return -ENODEV; >> + return -EINVAL; > > Sometimes changing errno can be viewed as ABI change and break applications. It seems that this is already breaking applications. iproute2 ip-link depends on the returned error: https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/iplink.c#n221 Cheers,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 73f2cbc440c9..b943336908a7 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3457,7 +3457,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, return rtnl_group_changelink(skb, net, nla_get_u32(tb[IFLA_GROUP]), ifm, extack, tb); - return -ENODEV; + return -EINVAL; } if (tb[IFLA_MAP] || tb[IFLA_PROTINFO])