[PATCH net-next] ip6_tunnel: enable to change proto of fb tunnels

[PATCH net-next] ip6_tunnel: enable to change proto of fb tunnels

[Nicolas Dichtel]

This is possible via the ioctl API:
> ip -6 tunnel change ip6tnl0 mode any

Let's align the netlink API:
> ip link set ip6tnl0 type ip6tnl mode any

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/ip6_tunnel.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 894d3158a6f0..03ad45189621 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -2053,8 +2053,17 @@ static int ip6_tnl_changelink(struct net_device *dev, struct nlattr *tb[],
 	struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id);
 	struct ip_tunnel_encap ipencap;
 
-	if (dev == ip6n->fb_tnl_dev)
-		return -EINVAL;
+	if (dev == ip6n->fb_tnl_dev) {
+		if (!data[IFLA_IPTUN_PROTO]) {
+			NL_SET_ERR_MSG(extack,
+				       "Only protocol can be changed for fallback tunnel");
+			return -EINVAL;
+		}
+
+		ip6_tnl_netlink_parms(data, &p);
+		ip6_tnl0_update(netdev_priv(ip6n->fb_tnl_dev), &p);
+		return 0;
+	}
 
 	if (ip_tunnel_netlink_encap_parms(data, &ipencap)) {
 		int err = ip6_tnl_encap_setup(t, &ipencap);
-- 
2.47.1

Re: [PATCH net-next] ip6_tunnel: enable to change proto of fb tunnels

[Simon Horman]

On Tue, Jun 17, 2025 at 06:01:25PM +0200, Nicolas Dichtel wrote:
> This is possible via the ioctl API:
> > ip -6 tunnel change ip6tnl0 mode any
> 
> Let's align the netlink API:
> > ip link set ip6tnl0 type ip6tnl mode any
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Thanks,

Looking over the code, I agree that this is supported by the ioctl
interface. And, for consistency, I think it makes sense for the netlink
interface to do so too.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next] ip6_tunnel: enable to change proto of fb tunnels

[Jakub Kicinski]

On Tue, 17 Jun 2025 18:01:25 +0200 Nicolas Dichtel wrote:
> +	if (dev == ip6n->fb_tnl_dev) {
> +		if (!data[IFLA_IPTUN_PROTO]) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Only protocol can be changed for fallback tunnel");
> +			return -EINVAL;
> +		}
> +
> +		ip6_tnl_netlink_parms(data, &p);
> +		ip6_tnl0_update(netdev_priv(ip6n->fb_tnl_dev), &p);
> +		return 0;

Hm, I guess its in line with old school netlink behavior where we'd
just toss unsupported attributes on the floor. But I wonder whether
it'd be better to explicitly reject other attrs?

Shouldn't be too painful with just one allowed:

	if (!data[IFLA_IPTUN_PROTO])
		goto ..

	ip6_tnl_netlink_parms(data, &p);

	data[IFLA_IPTUN_PROTO] = NULL;
	if (memchr_inv(data, 0, sizeof() * ARRAY_SIZE(ip6_tnl_policy)))
		goto ...

	ip6_tnl0_update(netdev_priv(ip6n->fb_tnl_dev), &p);

WDYT?

Re: [PATCH net-next] ip6_tunnel: enable to change proto of fb tunnels

[Nicolas Dichtel]

Le 20/06/2025 à 00:52, Jakub Kicinski a écrit :
> On Tue, 17 Jun 2025 18:01:25 +0200 Nicolas Dichtel wrote:
>> +	if (dev == ip6n->fb_tnl_dev) {
>> +		if (!data[IFLA_IPTUN_PROTO]) {
>> +			NL_SET_ERR_MSG(extack,
>> +				       "Only protocol can be changed for fallback tunnel");
>> +			return -EINVAL;
>> +		}
>> +
>> +		ip6_tnl_netlink_parms(data, &p);
>> +		ip6_tnl0_update(netdev_priv(ip6n->fb_tnl_dev), &p);
>> +		return 0;
> 
> Hm, I guess its in line with old school netlink behavior where we'd
> just toss unsupported attributes on the floor. But I wonder whether
> it'd be better to explicitly reject other attrs?

I tried to find a (simple) way to be strict but, by default 'ip link' dumps all
attributes and put them back in the message it sends.
Ie, with the command 'ip link set ip6tnl0 type ip6tnl mode any' all IFLA_IPTUN_*
attributes are set (to their current value) and only IFLA_IPTUN_PROTO has
another value.

> 
> Shouldn't be too painful with just one allowed:
> 
> 	if (!data[IFLA_IPTUN_PROTO])
> 		goto ..
> 
> 	ip6_tnl_netlink_parms(data, &p);
> 
> 	data[IFLA_IPTUN_PROTO] = NULL;
> 	if (memchr_inv(data, 0, sizeof() * ARRAY_SIZE(ip6_tnl_policy)))
> 		goto ...
> > 	ip6_tnl0_update(netdev_priv(ip6n->fb_tnl_dev), &p);
> 
> WDYT?
I already tried something similar, but it broke the 'ip link' command for the
reason explained above.
I was wondering if it's worth putting a lot of code to cover this case.
Any thoughts?


Regards,
Nicolas

Re: [PATCH net-next] ip6_tunnel: enable to change proto of fb tunnels

[Jakub Kicinski]

On Fri, 20 Jun 2025 10:14:26 +0200 Nicolas Dichtel wrote:
> > Hm, I guess its in line with old school netlink behavior where we'd
> > just toss unsupported attributes on the floor. But I wonder whether
> > it'd be better to explicitly reject other attrs?  
> 
> I tried to find a (simple) way to be strict but, by default 'ip link' dumps all
> attributes and put them back in the message it sends.
> Ie, with the command 'ip link set ip6tnl0 type ip6tnl mode any' all IFLA_IPTUN_*
> attributes are set (to their current value) and only IFLA_IPTUN_PROTO has
> another value.

Ah damn.

> > Shouldn't be too painful with just one allowed:
> > 
> > 	if (!data[IFLA_IPTUN_PROTO])
> > 		goto ..
> > 
> > 	ip6_tnl_netlink_parms(data, &p);
> > 
> > 	data[IFLA_IPTUN_PROTO] = NULL;
> > 	if (memchr_inv(data, 0, sizeof() * ARRAY_SIZE(ip6_tnl_policy)))
> > 		goto ...  
> > > 	ip6_tnl0_update(netdev_priv(ip6n->fb_tnl_dev), &p);  
> > 
> > WDYT?  
> I already tried something similar, but it broke the 'ip link' command for the
> reason explained above.
> I was wondering if it's worth putting a lot of code to cover this case.
> Any thoughts?

Yes, commit message and/or even a one-line comment in the code?