All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage
@ 2022-04-15 16:53 Florent Fourcot
  2022-04-15 16:53 ` [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given Florent Fourcot
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Florent Fourcot @ 2022-04-15 16:53 UTC (permalink / raw)
  To: netdev; +Cc: cong.wang, edumazet, Florent Fourcot

First commit forbids dangerous calls when both IFNAME and GROUP are
given, since it can introduce unexpected behaviour when IFNAME does not
match any interface.

Second patch achieves primary goal of this patchset to fix/improve
IFLA_ALT_IFNAME attribute, since previous code was never working for
newlink/setlink. ip-link command is probably getting interface index
before, and was not using this feature.

Last two patches are improving error code on corner cases.

Changes in v2:
  * Remove ifname argument in rtnl_dev_get/do_setlink
    functions (simplify code)
  * Use a boolean to avoid condition duplication in __rtnl_newlink

Changes in v3:
  * Simplify rtnl_dev_get signature

Changes in v4:
  * Rename link_lookup to link_specified

Changes in v5:
  * Re-order patches


Florent Fourcot (4):
  rtnetlink: return ENODEV when ifname does not exist and group is given
  rtnetlink: enable alt_ifname for setlink/newlink
  rtnetlink: return ENODEV when IFLA_ALT_IFNAME is used in dellink
  rtnetlink: return EINVAL when request cannot succeed

 net/core/rtnetlink.c | 91 ++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 46 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
@ 2022-04-15 16:53 ` Florent Fourcot
  2022-04-15 19:24   ` Stephen Hemminger
  2022-04-15 16:53 ` [PATCH v5 net-next 2/4] rtnetlink: enable alt_ifname for setlink/newlink Florent Fourcot
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Florent Fourcot @ 2022-04-15 16:53 UTC (permalink / raw)
  To: netdev; +Cc: cong.wang, edumazet, Florent Fourcot, Brian Baboch

When the interface does not exist, and a group is given, the given
parameters are being set to all interfaces of the given group. The given
IFNAME/ALT_IF_NAME are being ignored in that case.

That can be dangerous since a typo (or a deleted interface) can produce
weird side effects for caller:

Case 1:

 IFLA_IFNAME=valid_interface
 IFLA_GROUP=1
 MTU=1234

Case 1 will update MTU and group of the given interface "valid_interface".

Case 2:

 IFLA_IFNAME=doesnotexist
 IFLA_GROUP=1
 MTU=1234

Case 2 will update MTU of all interfaces in group 1. IFLA_IFNAME is
ignored in this case

This behaviour is not consistent and dangerous. In order to fix this issue,
we now return ENODEV when the given IFNAME does not exist.

Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr>
Signed-off-by: Brian Baboch <brian.baboch@wifirst.fr>
---
 net/core/rtnetlink.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 8bf770a7261e..2f538ab512d0 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3326,6 +3326,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct ifinfomsg *ifm;
 	char ifname[IFNAMSIZ];
 	struct nlattr **data;
+	bool link_specified;
 	int err;
 
 #ifdef CONFIG_MODULES
@@ -3346,12 +3347,16 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		ifname[0] = '\0';
 
 	ifm = nlmsg_data(nlh);
-	if (ifm->ifi_index > 0)
+	if (ifm->ifi_index > 0) {
+		link_specified = true;
 		dev = __dev_get_by_index(net, ifm->ifi_index);
-	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
+	} else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]) {
+		link_specified = true;
 		dev = rtnl_dev_get(net, NULL, tb[IFLA_ALT_IFNAME], ifname);
-	else
+	} else {
+		link_specified = false;
 		dev = NULL;
+	}
 
 	master_dev = NULL;
 	m_ops = NULL;
@@ -3454,7 +3459,12 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
-		if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
+		/* No dev found and NLM_F_CREATE not set. Requested dev does not exist,
+		 * or it's for a group
+		*/
+		if (link_specified)
+			return -ENODEV;
+		if (tb[IFLA_GROUP])
 			return rtnl_group_changelink(skb, net,
 						nla_get_u32(tb[IFLA_GROUP]),
 						ifm, extack, tb);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 net-next 2/4] rtnetlink: enable alt_ifname for setlink/newlink
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
  2022-04-15 16:53 ` [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given Florent Fourcot
@ 2022-04-15 16:53 ` Florent Fourcot
  2022-04-15 16:53 ` [PATCH v5 net-next 3/4] rtnetlink: return ENODEV when IFLA_ALT_IFNAME is used in dellink Florent Fourcot
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Florent Fourcot @ 2022-04-15 16:53 UTC (permalink / raw)
  To: netdev; +Cc: cong.wang, edumazet, Florent Fourcot, Jiri Pirko, Brian Baboch

buffer called "ifname" given in function rtnl_dev_get
is always valid when called by setlink/newlink,
but contains only empty string when IFLA_IFNAME is not given. So
IFLA_ALT_IFNAME is always ignored

This patch fixes rtnl_dev_get function with a remove of ifname argument,
and move ifname copy in do_setlink when required.

It extends feature of commit 76c9ac0ee878,
"net: rtnetlink: add possibility to use alternative names as message
handle""

CC: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr>
Signed-off-by: Brian Baboch <brian.baboch@wifirst.fr>
---
 net/core/rtnetlink.c | 69 +++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2f538ab512d0..5899b1d2de14 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2644,17 +2644,23 @@ static int do_set_proto_down(struct net_device *dev,
 static int do_setlink(const struct sk_buff *skb,
 		      struct net_device *dev, struct ifinfomsg *ifm,
 		      struct netlink_ext_ack *extack,
-		      struct nlattr **tb, char *ifname, int status)
+		      struct nlattr **tb, int status)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
+	char ifname[IFNAMSIZ];
 	int err;
 
 	err = validate_linkmsg(dev, tb, extack);
 	if (err < 0)
 		return err;
 
+	if (tb[IFLA_IFNAME])
+		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+	else
+		ifname[0] = '\0';
+
 	if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD] || tb[IFLA_TARGET_NETNSID]) {
-		const char *pat = ifname && ifname[0] ? ifname : NULL;
+		const char *pat = ifname[0] ? ifname : NULL;
 		struct net *net;
 		int new_ifindex;
 
@@ -3010,21 +3016,16 @@ static int do_setlink(const struct sk_buff *skb,
 }
 
 static struct net_device *rtnl_dev_get(struct net *net,
-				       struct nlattr *ifname_attr,
-				       struct nlattr *altifname_attr,
-				       char *ifname)
-{
-	char buffer[ALTIFNAMSIZ];
-
-	if (!ifname) {
-		ifname = buffer;
-		if (ifname_attr)
-			nla_strscpy(ifname, ifname_attr, IFNAMSIZ);
-		else if (altifname_attr)
-			nla_strscpy(ifname, altifname_attr, ALTIFNAMSIZ);
-		else
-			return NULL;
-	}
+				       struct nlattr *tb[])
+{
+	char ifname[ALTIFNAMSIZ];
+
+	if (tb[IFLA_IFNAME])
+		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+	else if (tb[IFLA_ALT_IFNAME])
+		nla_strscpy(ifname, tb[IFLA_ALT_IFNAME], ALTIFNAMSIZ);
+	else
+		return NULL;
 
 	return __dev_get_by_name(net, ifname);
 }
@@ -3037,7 +3038,6 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_device *dev;
 	int err;
 	struct nlattr *tb[IFLA_MAX+1];
-	char ifname[IFNAMSIZ];
 
 	err = nlmsg_parse_deprecated(nlh, sizeof(*ifm), tb, IFLA_MAX,
 				     ifla_policy, extack);
@@ -3048,17 +3048,12 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout;
 
-	if (tb[IFLA_IFNAME])
-		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
-	else
-		ifname[0] = '\0';
-
 	err = -EINVAL;
 	ifm = nlmsg_data(nlh);
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
-		dev = rtnl_dev_get(net, NULL, tb[IFLA_ALT_IFNAME], ifname);
+		dev = rtnl_dev_get(net, tb);
 	else
 		goto errout;
 
@@ -3067,7 +3062,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto errout;
 	}
 
-	err = do_setlink(skb, dev, ifm, extack, tb, ifname, 0);
+	err = do_setlink(skb, dev, ifm, extack, tb, 0);
 errout:
 	return err;
 }
@@ -3156,8 +3151,7 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(tgt_net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
-		dev = rtnl_dev_get(net, tb[IFLA_IFNAME],
-				   tb[IFLA_ALT_IFNAME], NULL);
+		dev = rtnl_dev_get(net, tb);
 	else if (tb[IFLA_GROUP])
 		err = rtnl_group_dellink(tgt_net, nla_get_u32(tb[IFLA_GROUP]));
 	else
@@ -3299,7 +3293,7 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
 
 	for_each_netdev_safe(net, dev, aux) {
 		if (dev->group == group) {
-			err = do_setlink(skb, dev, ifm, extack, tb, NULL, 0);
+			err = do_setlink(skb, dev, ifm, extack, tb, 0);
 			if (err < 0)
 				return err;
 		}
@@ -3341,18 +3335,13 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
-	if (tb[IFLA_IFNAME])
-		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
-	else
-		ifname[0] = '\0';
-
 	ifm = nlmsg_data(nlh);
 	if (ifm->ifi_index > 0) {
 		link_specified = true;
 		dev = __dev_get_by_index(net, ifm->ifi_index);
 	} else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]) {
 		link_specified = true;
-		dev = rtnl_dev_get(net, NULL, tb[IFLA_ALT_IFNAME], ifname);
+		dev = rtnl_dev_get(net, tb);
 	} else {
 		link_specified = false;
 		dev = NULL;
@@ -3455,7 +3444,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			status |= DO_SETLINK_NOTIFY;
 		}
 
-		return do_setlink(skb, dev, ifm, extack, tb, ifname, status);
+		return do_setlink(skb, dev, ifm, extack, tb, status);
 	}
 
 	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
@@ -3492,7 +3481,9 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!ops->alloc && !ops->setup)
 		return -EOPNOTSUPP;
 
-	if (!ifname[0]) {
+	if (tb[IFLA_IFNAME]) {
+		nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+	} else {
 		snprintf(ifname, IFNAMSIZ, "%s%%d", ops->kind);
 		name_assign_type = NET_NAME_ENUM;
 	}
@@ -3664,8 +3655,7 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(tgt_net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
-		dev = rtnl_dev_get(tgt_net, tb[IFLA_IFNAME],
-				   tb[IFLA_ALT_IFNAME], NULL);
+		dev = rtnl_dev_get(tgt_net, tb);
 	else
 		goto out;
 
@@ -3760,8 +3750,7 @@ static int rtnl_linkprop(int cmd, struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
-		dev = rtnl_dev_get(net, tb[IFLA_IFNAME],
-				   tb[IFLA_ALT_IFNAME], NULL);
+		dev = rtnl_dev_get(net, tb);
 	else
 		return -EINVAL;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 net-next 3/4] rtnetlink: return ENODEV when IFLA_ALT_IFNAME is used in dellink
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
  2022-04-15 16:53 ` [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given Florent Fourcot
  2022-04-15 16:53 ` [PATCH v5 net-next 2/4] rtnetlink: enable alt_ifname for setlink/newlink Florent Fourcot
@ 2022-04-15 16:53 ` Florent Fourcot
  2022-04-15 16:53 ` [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed Florent Fourcot
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Florent Fourcot @ 2022-04-15 16:53 UTC (permalink / raw)
  To: netdev; +Cc: cong.wang, edumazet, Florent Fourcot, Jiri Pirko, Brian Baboch

If IFLA_ALT_IFNAME is set and given interface is not found,
we should return ENODEV and be consistent with IFLA_IFNAME
behaviour
This commit extends feature of commit 76c9ac0ee878,
"net: rtnetlink: add possibility to use alternative names as message handle"

CC: Jiri Pirko <jiri@mellanox.com>
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 5899b1d2de14..73f2cbc440c9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3158,7 +3158,7 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 
 	if (!dev) {
-		if (tb[IFLA_IFNAME] || ifm->ifi_index > 0)
+		if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME] || ifm->ifi_index > 0)
 			err = -ENODEV;
 
 		goto out;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
                   ` (2 preceding siblings ...)
  2022-04-15 16:53 ` [PATCH v5 net-next 3/4] rtnetlink: return ENODEV when IFLA_ALT_IFNAME is used in dellink Florent Fourcot
@ 2022-04-15 16:53 ` Florent Fourcot
  2022-04-15 19:26   ` Stephen Hemminger
  2022-04-15 19:03 ` [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Jakub Kicinski
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Florent Fourcot @ 2022-04-15 16:53 UTC (permalink / raw)
  To: netdev; +Cc: cong.wang, edumazet, Florent Fourcot, Brian Baboch

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;
 	}
 
 	if (tb[IFLA_MAP] || tb[IFLA_PROTINFO])
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
                   ` (3 preceding siblings ...)
  2022-04-15 16:53 ` [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed Florent Fourcot
@ 2022-04-15 19:03 ` Jakub Kicinski
  2022-04-19 11:50 ` patchwork-bot+netdevbpf
  2022-04-19 11:50 ` Jason A. Donenfeld
  6 siblings, 0 replies; 19+ messages in thread
From: Jakub Kicinski @ 2022-04-15 19:03 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, cong.wang, edumazet

On Fri, 15 Apr 2022 18:53:26 +0200 Florent Fourcot wrote:
> First commit forbids dangerous calls when both IFNAME and GROUP are
> given, since it can introduce unexpected behaviour when IFNAME does not
> match any interface.
> 
> Second patch achieves primary goal of this patchset to fix/improve
> IFLA_ALT_IFNAME attribute, since previous code was never working for
> newlink/setlink. ip-link command is probably getting interface index
> before, and was not using this feature.
> 
> Last two patches are improving error code on corner cases.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given
  2022-04-15 16:53 ` [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given Florent Fourcot
@ 2022-04-15 19:24   ` Stephen Hemminger
  2022-04-19  7:29     ` Florent Fourcot
  0 siblings, 1 reply; 19+ messages in thread
From: Stephen Hemminger @ 2022-04-15 19:24 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, cong.wang, edumazet, Brian Baboch

On Fri, 15 Apr 2022 18:53:27 +0200
Florent Fourcot <florent.fourcot@wifirst.fr> wrote:

>  	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
> -		if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
> +		/* No dev found and NLM_F_CREATE not set. Requested dev does not exist,
> +		 * or it's for a group
> +		*/
> +		if (link_specified)
> +			return -ENODEV;

Please add extack error message as well?
Simple errno's are harder to debug.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed
  2022-04-15 16:53 ` [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed Florent Fourcot
@ 2022-04-15 19:26   ` Stephen Hemminger
  2022-04-19  7:29     ` Florent Fourcot
  2022-04-22  1:27     ` Vinicius Costa Gomes
  0 siblings, 2 replies; 19+ messages in thread
From: Stephen Hemminger @ 2022-04-15 19:26 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, cong.wang, edumazet, Brian Baboch

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given
  2022-04-15 19:24   ` Stephen Hemminger
@ 2022-04-19  7:29     ` Florent Fourcot
  2022-04-19 15:04       ` Stephen Hemminger
  0 siblings, 1 reply; 19+ messages in thread
From: Florent Fourcot @ 2022-04-19  7:29 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, cong.wang, edumazet, Brian Baboch

Hello,


>> +		if (link_specified)
>> +			return -ENODEV;
> 
> Please add extack error message as well?
> Simple errno's are harder to debug.


What kind of message have you in mind for that one? Something like 
"Interface not found" does not have extra information for ENODEV code.

At this place, one gave interface index or interface name, and nothing 
matched.


Thanks,

-- 
Florent Fourcot.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed
  2022-04-15 19:26   ` Stephen Hemminger
@ 2022-04-19  7:29     ` Florent Fourcot
  2022-04-22  1:27     ` Vinicius Costa Gomes
  1 sibling, 0 replies; 19+ messages in thread
From: Florent Fourcot @ 2022-04-19  7:29 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, cong.wang, edumazet, Brian Baboch


>> -		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.

-- 
Florent Fourcot.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
                   ` (4 preceding siblings ...)
  2022-04-15 19:03 ` [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Jakub Kicinski
@ 2022-04-19 11:50 ` patchwork-bot+netdevbpf
  2022-04-19 11:50 ` Jason A. Donenfeld
  6 siblings, 0 replies; 19+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-19 11:50 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, cong.wang, edumazet

Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 15 Apr 2022 18:53:26 +0200 you wrote:
> First commit forbids dangerous calls when both IFNAME and GROUP are
> given, since it can introduce unexpected behaviour when IFNAME does not
> match any interface.
> 
> Second patch achieves primary goal of this patchset to fix/improve
> IFLA_ALT_IFNAME attribute, since previous code was never working for
> newlink/setlink. ip-link command is probably getting interface index
> before, and was not using this feature.
> 
> [...]

Here is the summary with links:
  - [v5,net-next,1/4] rtnetlink: return ENODEV when ifname does not exist and group is given
    https://git.kernel.org/netdev/net-next/c/ef2a7c9065ce
  - [v5,net-next,2/4] rtnetlink: enable alt_ifname for setlink/newlink
    https://git.kernel.org/netdev/net-next/c/5ea08b5286f6
  - [v5,net-next,3/4] rtnetlink: return ENODEV when IFLA_ALT_IFNAME is used in dellink
    https://git.kernel.org/netdev/net-next/c/dee04163e9f2
  - [v5,net-next,4/4] rtnetlink: return EINVAL when request cannot succeed
    https://git.kernel.org/netdev/net-next/c/b6177d3240a4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage
  2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
                   ` (5 preceding siblings ...)
  2022-04-19 11:50 ` patchwork-bot+netdevbpf
@ 2022-04-19 11:50 ` Jason A. Donenfeld
  2022-04-19 12:51   ` [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed" Florent Fourcot
  2022-04-19 14:25   ` [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
  6 siblings, 2 replies; 19+ messages in thread
From: Jason A. Donenfeld @ 2022-04-19 11:50 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, cong.wang, edumazet

Hi Florent,

On Fri, Apr 15, 2022 at 06:53:26PM +0200, Florent Fourcot wrote:
> First commit forbids dangerous calls when both IFNAME and GROUP are
> given, since it can introduce unexpected behaviour when IFNAME does not
> match any interface.
> 
> Second patch achieves primary goal of this patchset to fix/improve
> IFLA_ALT_IFNAME attribute, since previous code was never working for
> newlink/setlink. ip-link command is probably getting interface index
> before, and was not using this feature.
> 
> Last two patches are improving error code on corner cases.

This was just merged to net-next and appears to have broken the
wireguard test suite over on https://build.wireguard.com/

[+] Launching tests...
[    0.796066] init.sh (28) used greatest stack depth: 29152 bytes left
[    0.803809] ip (29) used greatest stack depth: 28544 bytes left
[+] ip netns add wg-test-27-0
[+] ip netns add wg-test-27-1
[    0.842841] ip (32) used greatest stack depth: 27512 bytes left
[+] ip netns add wg-test-27-2
[+] NS0: ip link set up dev lo
[+] NS0: ip link add dev wg0 type wireguard
[    0.896074] ip (35) used greatest stack depth: 27152 bytes left
Command "add" is unknown, try "ip link help".
[+] NS0: ip link del dev wg0
[+] NS0: ip link del dev wg1
[+] NS1: ip link del dev wg0
[+] NS1: ip link del dev wg1
[+] NS2: ip link del dev wg0
[+] NS2: ip link del dev wg1
[+] ip netns del wg-test-27-1
[+] ip netns del wg-test-27-2
[+] ip netns del wg-test-27-0
[-] Tests failed with exit code 255! ☹

So apparently something goes wrong with "ip link add dev wg0 type
wireguard". Not quite sure what yet, though. You can try it for yourself
with:

    make -C tools/testing/selftests/wireguard/qemu -j$(nproc)

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed"
  2022-04-19 11:50 ` Jason A. Donenfeld
@ 2022-04-19 12:51   ` Florent Fourcot
  2022-04-20 14:48     ` Guillaume Nault
                       ` (2 more replies)
  2022-04-19 14:25   ` [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
  1 sibling, 3 replies; 19+ messages in thread
From: Florent Fourcot @ 2022-04-19 12:51 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, Brian Baboch, Florent Fourcot

This reverts commit b6177d3240a4

ip-link command is testing kernel capability by sending a RTM_NEWLINK
request, without any argument. It accepts everything in reply, except
EOPNOTSUPP and EINVAL (functions iplink_have_newlink / accept_msg)

So we must keep compatiblity here, invalid empty message should not
return EINVAL

Signed-off-by: Florent Fourcot <florent.fourcot@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 b943336908a7..73f2cbc440c9 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 -EINVAL;
+		return -ENODEV;
 	}
 
 	if (tb[IFLA_MAP] || tb[IFLA_PROTINFO])
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage
  2022-04-19 11:50 ` Jason A. Donenfeld
  2022-04-19 12:51   ` [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed" Florent Fourcot
@ 2022-04-19 14:25   ` Florent Fourcot
  1 sibling, 0 replies; 19+ messages in thread
From: Florent Fourcot @ 2022-04-19 14:25 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: netdev, cong.wang, edumazet

Hello Jason,

Thanks for the report. Stephen was right, and I introduced a regression 
on ip-link.

I submitted a revert patch with more context on what happened. I'm very 
sorry for that.

-- 
Florent Fourcot.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given
  2022-04-19  7:29     ` Florent Fourcot
@ 2022-04-19 15:04       ` Stephen Hemminger
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Hemminger @ 2022-04-19 15:04 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, cong.wang, edumazet, Brian Baboch

On Tue, 19 Apr 2022 09:29:37 +0200
Florent Fourcot <florent.fourcot@wifirst.fr> wrote:

> Hello,
> 
> 
> >> +		if (link_specified)
> >> +			return -ENODEV;  
> > 
> > Please add extack error message as well?
> > Simple errno's are harder to debug.  
> 
> 
> What kind of message have you in mind for that one? Something like 
> "Interface not found" does not have extra information for ENODEV code.
> 
> At this place, one gave interface index or interface name, and nothing 
> matched.

Not sure how code gets here. Maybe "interface name required"

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed"
  2022-04-19 12:51   ` [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed" Florent Fourcot
@ 2022-04-20 14:48     ` Guillaume Nault
  2022-04-21  1:15     ` Eric Dumazet
  2022-04-22 10:10     ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 19+ messages in thread
From: Guillaume Nault @ 2022-04-20 14:48 UTC (permalink / raw)
  To: Florent Fourcot, David Miller, Jakub Kicinski, Paolo Abeni
  Cc: netdev, Stephen Hemminger, Brian Baboch

On Tue, Apr 19, 2022 at 02:51:51PM +0200, Florent Fourcot wrote:
> This reverts commit b6177d3240a4
> 
> ip-link command is testing kernel capability by sending a RTM_NEWLINK
> request, without any argument. It accepts everything in reply, except
> EOPNOTSUPP and EINVAL (functions iplink_have_newlink / accept_msg)
> 
> So we must keep compatiblity here, invalid empty message should not
> return EINVAL

"ip link" is currently unusable on net-next without this patch.
Can we please rush this fix in?

Tested-by: Guillaume Nault <gnault@redhat.com>
Fixes: b6177d3240a4 ("rtnetlink: return EINVAL when request cannot succeed")

> Signed-off-by: Florent Fourcot <florent.fourcot@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 b943336908a7..73f2cbc440c9 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 -EINVAL;
> +		return -ENODEV;
>  	}
>  
>  	if (tb[IFLA_MAP] || tb[IFLA_PROTINFO])
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed"
  2022-04-19 12:51   ` [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed" Florent Fourcot
  2022-04-20 14:48     ` Guillaume Nault
@ 2022-04-21  1:15     ` Eric Dumazet
  2022-04-22 10:10     ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2022-04-21  1:15 UTC (permalink / raw)
  To: Florent Fourcot, netdev; +Cc: Stephen Hemminger, Brian Baboch


On 4/19/22 05:51, Florent Fourcot wrote:
> This reverts commit b6177d3240a4
>
> ip-link command is testing kernel capability by sending a RTM_NEWLINK
> request, without any argument. It accepts everything in reply, except
> EOPNOTSUPP and EINVAL (functions iplink_have_newlink / accept_msg)
>
> So we must keep compatiblity here, invalid empty message should not
> return EINVAL
>
> Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr>


Reviewed-by: Eric Dumazet <edumazet@google.com>


Thanks for fixing this issue.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed
  2022-04-15 19:26   ` Stephen Hemminger
  2022-04-19  7:29     ` Florent Fourcot
@ 2022-04-22  1:27     ` Vinicius Costa Gomes
  1 sibling, 0 replies; 19+ messages in thread
From: Vinicius Costa Gomes @ 2022-04-22  1:27 UTC (permalink / raw)
  To: Stephen Hemminger, Florent Fourcot
  Cc: netdev, cong.wang, edumazet, Brian Baboch

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,
-- 
Vinicius

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed"
  2022-04-19 12:51   ` [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed" Florent Fourcot
  2022-04-20 14:48     ` Guillaume Nault
  2022-04-21  1:15     ` Eric Dumazet
@ 2022-04-22 10:10     ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 19+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-22 10:10 UTC (permalink / raw)
  To: Florent Fourcot; +Cc: netdev, stephen, brian.baboch

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 19 Apr 2022 14:51:51 +0200 you wrote:
> This reverts commit b6177d3240a4
> 
> ip-link command is testing kernel capability by sending a RTM_NEWLINK
> request, without any argument. It accepts everything in reply, except
> EOPNOTSUPP and EINVAL (functions iplink_have_newlink / accept_msg)
> 
> So we must keep compatiblity here, invalid empty message should not
> return EINVAL
> 
> [...]

Here is the summary with links:
  - [net-next] Revert "rtnetlink: return EINVAL when request cannot succeed"
    https://git.kernel.org/netdev/net-next/c/6f37c9f9dfbf

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2022-04-22 10:10 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 16:53 [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot
2022-04-15 16:53 ` [PATCH v5 net-next 1/4] rtnetlink: return ENODEV when ifname does not exist and group is given Florent Fourcot
2022-04-15 19:24   ` Stephen Hemminger
2022-04-19  7:29     ` Florent Fourcot
2022-04-19 15:04       ` Stephen Hemminger
2022-04-15 16:53 ` [PATCH v5 net-next 2/4] rtnetlink: enable alt_ifname for setlink/newlink Florent Fourcot
2022-04-15 16:53 ` [PATCH v5 net-next 3/4] rtnetlink: return ENODEV when IFLA_ALT_IFNAME is used in dellink Florent Fourcot
2022-04-15 16:53 ` [PATCH v5 net-next 4/4] rtnetlink: return EINVAL when request cannot succeed Florent Fourcot
2022-04-15 19:26   ` Stephen Hemminger
2022-04-19  7:29     ` Florent Fourcot
2022-04-22  1:27     ` Vinicius Costa Gomes
2022-04-15 19:03 ` [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Jakub Kicinski
2022-04-19 11:50 ` patchwork-bot+netdevbpf
2022-04-19 11:50 ` Jason A. Donenfeld
2022-04-19 12:51   ` [PATCH net-next] Revert "rtnetlink: return EINVAL when request cannot succeed" Florent Fourcot
2022-04-20 14:48     ` Guillaume Nault
2022-04-21  1:15     ` Eric Dumazet
2022-04-22 10:10     ` patchwork-bot+netdevbpf
2022-04-19 14:25   ` [PATCH v5 net-next 0/4] rtnetlink: improve ALT_IFNAME config and fix dangerous GROUP usage Florent Fourcot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.