[PATCH net 0/2] net: notify users when an iface cannot change its netns

[PATCH net 0/2] net: notify users when an iface cannot change its netns

[Nicolas Dichtel]

This series adds a way to see if an interface cannot be moved to another netns.
I target this series to 'net' because this was possible until commit
05c1280a2bcf ("netdev_features: convert NETIF_F_NETNS_LOCAL to dev->netns_local").

 include/uapi/linux/if_link.h | 1 +
 net/core/rtnetlink.c         | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments are welcome.

Regards,
Nicolas

[PATCH net 1/2] net: advertise 'netns local' property via netlink

[Nicolas Dichtel]

Since the below commit, there is no way to see if the netns_local property
is set on a device. Let's add a netlink attribute to advertise it.

CC: stable@vger.kernel.org
Fixes: 05c1280a2bcf ("netdev_features: convert NETIF_F_NETNS_LOCAL to dev->netns_local")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/uapi/linux/if_link.h | 1 +
 net/core/rtnetlink.c         | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bfe880fbbb24..ed4a64e1c8f1 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -378,6 +378,7 @@ enum {
 	IFLA_GRO_IPV4_MAX_SIZE,
 	IFLA_DPLL_PIN,
 	IFLA_MAX_PACING_OFFLOAD_HORIZON,
+	IFLA_NETNS_LOCAL,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d1e559fce918..5032e65b8faa 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1287,6 +1287,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(4) /* IFLA_TSO_MAX_SEGS */
 	       + nla_total_size(1) /* IFLA_OPERSTATE */
 	       + nla_total_size(1) /* IFLA_LINKMODE */
+	       + nla_total_size(1) /* IFLA_NETNS_LOCAL */
 	       + nla_total_size(4) /* IFLA_CARRIER_CHANGES */
 	       + nla_total_size(4) /* IFLA_LINK_NETNSID */
 	       + nla_total_size(4) /* IFLA_GROUP */
@@ -2041,6 +2042,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 		       netif_running(dev) ? READ_ONCE(dev->operstate) :
 					    IF_OPER_DOWN) ||
 	    nla_put_u8(skb, IFLA_LINKMODE, READ_ONCE(dev->link_mode)) ||
+	    nla_put_u8(skb, IFLA_NETNS_LOCAL, dev->netns_local) ||
 	    nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
 	    nla_put_u32(skb, IFLA_MIN_MTU, READ_ONCE(dev->min_mtu)) ||
 	    nla_put_u32(skb, IFLA_MAX_MTU, READ_ONCE(dev->max_mtu)) ||
@@ -2229,6 +2231,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_ALLMULTI]		= { .type = NLA_REJECT },
 	[IFLA_GSO_IPV4_MAX_SIZE]	= NLA_POLICY_MIN(NLA_U32, MAX_TCP_HEADER + 1),
 	[IFLA_GRO_IPV4_MAX_SIZE]	= { .type = NLA_U32 },
+	[IFLA_NETNS_LOCAL]	= { .type = NLA_U8 },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
-- 
2.47.1

[PATCH net 2/2] net: enhance error message for 'netns local' iface

[Nicolas Dichtel]

The current message is "Invalid argument". Let's help the user by
explaining the error.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5032e65b8faa..91b358bdfe5c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3024,8 +3024,12 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
 		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
 
 		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex);
-		if (err)
+		if (err) {
+			if (dev->netns_local)
+				NL_SET_ERR_MSG(extack,
+					       "The interface has the 'netns local' property");
 			goto errout;
+		}
 
 		status |= DO_SETLINK_MODIFIED;
 	}
-- 
2.47.1

Re: [PATCH net 1/2] net: advertise 'netns local' property via netlink

[Eric Dumazet]

On Thu, Feb 6, 2025 at 5:51 PM Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
>
> Since the below commit, there is no way to see if the netns_local property
> is set on a device. Let's add a netlink attribute to advertise it.
>


> CC: stable@vger.kernel.org
> Fixes: 05c1280a2bcf ("netdev_features: convert NETIF_F_NETNS_LOCAL to dev->netns_local")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  include/uapi/linux/if_link.h | 1 +
>  net/core/rtnetlink.c         | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index bfe880fbbb24..ed4a64e1c8f1 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -378,6 +378,7 @@ enum {
>         IFLA_GRO_IPV4_MAX_SIZE,
>         IFLA_DPLL_PIN,
>         IFLA_MAX_PACING_OFFLOAD_HORIZON,
> +       IFLA_NETNS_LOCAL,
>         __IFLA_MAX
>  };
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index d1e559fce918..5032e65b8faa 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1287,6 +1287,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
>                + nla_total_size(4) /* IFLA_TSO_MAX_SEGS */
>                + nla_total_size(1) /* IFLA_OPERSTATE */
>                + nla_total_size(1) /* IFLA_LINKMODE */
> +              + nla_total_size(1) /* IFLA_NETNS_LOCAL */
>                + nla_total_size(4) /* IFLA_CARRIER_CHANGES */
>                + nla_total_size(4) /* IFLA_LINK_NETNSID */
>                + nla_total_size(4) /* IFLA_GROUP */
> @@ -2041,6 +2042,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>                        netif_running(dev) ? READ_ONCE(dev->operstate) :
>                                             IF_OPER_DOWN) ||
>             nla_put_u8(skb, IFLA_LINKMODE, READ_ONCE(dev->link_mode)) ||
> +           nla_put_u8(skb, IFLA_NETNS_LOCAL, dev->netns_local) ||
>             nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
>             nla_put_u32(skb, IFLA_MIN_MTU, READ_ONCE(dev->min_mtu)) ||
>             nla_put_u32(skb, IFLA_MAX_MTU, READ_ONCE(dev->max_mtu)) ||
> @@ -2229,6 +2231,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>         [IFLA_ALLMULTI]         = { .type = NLA_REJECT },
>         [IFLA_GSO_IPV4_MAX_SIZE]        = NLA_POLICY_MIN(NLA_U32, MAX_TCP_HEADER + 1),
>         [IFLA_GRO_IPV4_MAX_SIZE]        = { .type = NLA_U32 },
> +       [IFLA_NETNS_LOCAL]      = { .type = NLA_U8 },

As this is a read-only attribute, I would suggest NLA_REJECT

Re: [PATCH net 1/2] net: advertise 'netns local' property via netlink

[Ido Schimmel]

On Thu, Feb 06, 2025 at 05:59:03PM +0100, Eric Dumazet wrote:
> On Thu, Feb 6, 2025 at 5:51 PM Nicolas Dichtel
> > @@ -2229,6 +2231,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
> >         [IFLA_ALLMULTI]         = { .type = NLA_REJECT },
> >         [IFLA_GSO_IPV4_MAX_SIZE]        = NLA_POLICY_MIN(NLA_U32, MAX_TCP_HEADER + 1),
> >         [IFLA_GRO_IPV4_MAX_SIZE]        = { .type = NLA_U32 },
> > +       [IFLA_NETNS_LOCAL]      = { .type = NLA_U8 },
> 
> As this is a read-only attribute, I would suggest NLA_REJECT

And please update the spec:
Documentation/netlink/specs/rt_link.yaml

Re: [PATCH net 2/2] net: enhance error message for 'netns local' iface

[Andrew Lunn]

On Thu, Feb 06, 2025 at 05:50:27PM +0100, Nicolas Dichtel wrote:
> The current message is "Invalid argument". Let's help the user by
> explaining the error.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/core/rtnetlink.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5032e65b8faa..91b358bdfe5c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3024,8 +3024,12 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
>  		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
>  
>  		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex);
> -		if (err)
> +		if (err) {
> +			if (dev->netns_local)
> +				NL_SET_ERR_MSG(extack,
> +					       "The interface has the 'netns local' property");

This seems to have the wrong order. Why even try calling
__dev_change_net_namespace() if you know it is going to fail?

Maybe this NL_SET_ERR_MSG() should be pushed into
__dev_change_net_namespace()? You could then return useful messages if
the altnames conflict, the ifindex is already in use, etc.

	Andrew

Re: [PATCH net 1/2] net: advertise 'netns local' property via netlink

[Jakub Kicinski]

On Thu,  6 Feb 2025 17:50:26 +0100 Nicolas Dichtel wrote:
> Since the below commit, there is no way to see if the netns_local property
> is set on a device. Let's add a netlink attribute to advertise it.

I think the motivation for the change may be worth elaborating on.
It's a bit unclear to me what user space would care about this
information, a bit of a "story" on how you hit the issue could
be useful perhaps? The uAPI is new but the stable tag indicates
regression..

> @@ -2041,6 +2042,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>  		       netif_running(dev) ? READ_ONCE(dev->operstate) :
>  					    IF_OPER_DOWN) ||
>  	    nla_put_u8(skb, IFLA_LINKMODE, READ_ONCE(dev->link_mode)) ||
> +	    nla_put_u8(skb, IFLA_NETNS_LOCAL, dev->netns_local) ||

Maybe nla_put_flag() ? Or do you really care about false being there?
The 3 bytes wasted on padding always makes me question when people pick
NLA_u8.

>  	    nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
>  	    nla_put_u32(skb, IFLA_MIN_MTU, READ_ONCE(dev->min_mtu)) ||
>  	    nla_put_u32(skb, IFLA_MAX_MTU, READ_ONCE(dev->max_mtu)) ||

Re: [PATCH net 2/2] net: enhance error message for 'netns local' iface

[Nicolas Dichtel]

Le 07/02/2025 à 00:02, Andrew Lunn a écrit :
> On Thu, Feb 06, 2025 at 05:50:27PM +0100, Nicolas Dichtel wrote:
>> The current message is "Invalid argument". Let's help the user by
>> explaining the error.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>  net/core/rtnetlink.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 5032e65b8faa..91b358bdfe5c 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -3024,8 +3024,12 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
>>  		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
>>  
>>  		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex);
>> -		if (err)
>> +		if (err) {
>> +			if (dev->netns_local)
>> +				NL_SET_ERR_MSG(extack,
>> +					       "The interface has the 'netns local' property");
> 
> This seems to have the wrong order. Why even try calling
> __dev_change_net_namespace() if you know it is going to fail?
> 
> Maybe this NL_SET_ERR_MSG() should be pushed into
> __dev_change_net_namespace()? You could then return useful messages if
> the altnames conflict, the ifindex is already in use, etc.
Users of dev_change_net_namespace() are not netlink users, so I kept the same
API. I will plumb the extack into __dev_change_net_namespace().


Thanks,
Nicolas

Re: [PATCH net 1/2] net: advertise 'netns local' property via netlink

[Nicolas Dichtel]

Le 07/02/2025 à 00:39, Jakub Kicinski a écrit :
> On Thu,  6 Feb 2025 17:50:26 +0100 Nicolas Dichtel wrote:
>> Since the below commit, there is no way to see if the netns_local property
>> is set on a device. Let's add a netlink attribute to advertise it.
> 
> I think the motivation for the change may be worth elaborating on.
> It's a bit unclear to me what user space would care about this
> information, a bit of a "story" on how you hit the issue could
> be useful perhaps? The uAPI is new but the stable tag indicates
> regression..
To make it short: we were trying a new NIC with a custom distro provided by a
vendor (with out of tree drivers). We were unable to move the interface in
another netns. Thanks to ethtool we were able to confirm that the 'netns-local'
flag was set. Having this information helps debugging.

> 
>> @@ -2041,6 +2042,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>>  		       netif_running(dev) ? READ_ONCE(dev->operstate) :
>>  					    IF_OPER_DOWN) ||
>>  	    nla_put_u8(skb, IFLA_LINKMODE, READ_ONCE(dev->link_mode)) ||
>> +	    nla_put_u8(skb, IFLA_NETNS_LOCAL, dev->netns_local) ||
> 
> Maybe nla_put_flag() ? Or do you really care about false being there?
It depends if the commit is backported or not. If it won't be backported, having
the false value helps to know that the kernel support this attribute (and so
that the property is not set).

FWIW, I will be off for one week, I will come back to this later.

> The 3 bytes wasted on padding always makes me question when people pick
> NLA_u8.
> 
>>  	    nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
>>  	    nla_put_u32(skb, IFLA_MIN_MTU, READ_ONCE(dev->min_mtu)) ||
>>  	    nla_put_u32(skb, IFLA_MAX_MTU, READ_ONCE(dev->max_mtu)) ||

Re: [PATCH net 1/2] net: advertise 'netns local' property via netlink

[Jakub Kicinski]

On Fri, 7 Feb 2025 10:10:49 +0100 Nicolas Dichtel wrote:
> Le 07/02/2025 à 00:39, Jakub Kicinski a écrit :
> > On Thu,  6 Feb 2025 17:50:26 +0100 Nicolas Dichtel wrote:  
> >> Since the below commit, there is no way to see if the netns_local property
> >> is set on a device. Let's add a netlink attribute to advertise it.  
> > 
> > I think the motivation for the change may be worth elaborating on.
> > It's a bit unclear to me what user space would care about this
> > information, a bit of a "story" on how you hit the issue could
> > be useful perhaps? The uAPI is new but the stable tag indicates
> > regression..  
> To make it short: we were trying a new NIC with a custom distro provided by a
> vendor (with out of tree drivers). We were unable to move the interface in
> another netns. Thanks to ethtool we were able to confirm that the 'netns-local'
> flag was set. Having this information helps debugging.

Thanks, makes sense. Still a bit unsure if this is a stable candidate,
if you don't mind net-next that'd be my preference. If you do mind,
I'll live with it :)

> >> @@ -2041,6 +2042,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
> >>  		       netif_running(dev) ? READ_ONCE(dev->operstate) :
> >>  					    IF_OPER_DOWN) ||
> >>  	    nla_put_u8(skb, IFLA_LINKMODE, READ_ONCE(dev->link_mode)) ||
> >> +	    nla_put_u8(skb, IFLA_NETNS_LOCAL, dev->netns_local) ||  
> > 
> > Maybe nla_put_flag() ? Or do you really care about false being there?  
> It depends if the commit is backported or not. If it won't be backported, having
> the false value helps to know that the kernel support this attribute (and so
> that the property is not set).

Wish we had a good solution for this, it's always the argument against
flags :(