diff mbox series

[v3,net-next,3/4] rtnetlink: return ENODEV when ifname does not exist and group is given

Message ID 20220405134237.16533-3-florent.fourcot@wifirst.fr (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [v3,net-next,1/4] rtnetlink: enable alt_ifname for setlink/newlink | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter warning Series does not have a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers fail 4 maintainers not CCed: davem@davemloft.net kuba@kernel.org petrm@nvidia.com pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch warning CHECK: Please don't use multiple blank lines WARNING: Block comments should align the * on each line WARNING: line length of 87 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Florent Fourcot April 5, 2022, 1:42 p.m. UTC 
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.

Changes in v3:
  * Use a boolean to have condition duplication

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(-)

Comments

Jakub Kicinski April 6, 2022, 12:47 a.m. UTC | #1 
On Tue,  5 Apr 2022 15:42:36 +0200 Florent Fourcot wrote:
> Changes in v3:
>   * Use a boolean to have condition duplication
> 
> 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 33dbeed7e531..e93f4058cf08 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3279,6 +3279,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	struct net *dest_net, *link_net;
>  	struct nlattr **slave_data;
>  	char kind[MODULE_NAME_LEN];
> +	bool link_lookup = false;

link_specified ? Somehow link_lookup does not speak to me.

>  	struct net_device *dev;
>  	struct ifinfomsg *ifm;
>  	char ifname[IFNAMSIZ];
> @@ -3298,12 +3299,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return err;
>  
>  	ifm = nlmsg_data(nlh);
> -	if (ifm->ifi_index > 0)
> +	if (ifm->ifi_index > 0) {
> +		link_lookup = 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_lookup = true;
>  		dev = rtnl_dev_get(net, tb);
> -	else
> +	} else {
>  		dev = NULL;

maybe set it to false here and don't do the initialization.
It doesn't matter in practice but the function has a "retry"
goto which skips the init.

> +	}
>  
>  	master_dev = NULL;
>  	m_ops = NULL;
> @@ -3405,8 +3409,14 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return do_setlink(skb, dev, ifm, extack, tb, status);
>  	}
>  
> +

spurious new line

>  	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_lookup)
> +			return -ENODEV;
> +		if (tb[IFLA_GROUP])
diff mbox series

Patch

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 33dbeed7e531..e93f4058cf08 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3279,6 +3279,7 @@  static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net *dest_net, *link_net;
 	struct nlattr **slave_data;
 	char kind[MODULE_NAME_LEN];
+	bool link_lookup = false;
 	struct net_device *dev;
 	struct ifinfomsg *ifm;
 	char ifname[IFNAMSIZ];
@@ -3298,12 +3299,15 @@  static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return err;
 
 	ifm = nlmsg_data(nlh);
-	if (ifm->ifi_index > 0)
+	if (ifm->ifi_index > 0) {
+		link_lookup = 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_lookup = true;
 		dev = rtnl_dev_get(net, tb);
-	else
+	} else {
 		dev = NULL;
+	}
 
 	master_dev = NULL;
 	m_ops = NULL;
@@ -3405,8 +3409,14 @@  static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return do_setlink(skb, dev, ifm, extack, tb, status);
 	}
 
+
 	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_lookup)
+			return -ENODEV;
+		if (tb[IFLA_GROUP])
 			return rtnl_group_changelink(skb, net,
 						nla_get_u32(tb[IFLA_GROUP]),
 						ifm, extack, tb);