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

[Jakub Kicinski <kuba@kernel.org>]

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