| Message ID | 20241108092555.5714-1-emanuele.santini.88@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipv6: fix the address length for net_device on a GRE tunnel | expand |
On Fri, Nov 08, 2024 at 10:25:55AM +0100, Emanuele Santini wrote: > While GRE tunneling does not require > a hardware address, a random Ethernet address is still assigned to > the 'net_device'. That's really surprising and not what I can see on my system. Are you really talking about ip6gre (and not ip6gretap)? > Therefore, the correct 'addr_len' value should be > the size of an Ethernet address (6 bytes), not the size of an IPv6 > address. Ethernet address length only makes sense for ip6gretap. This doesn't seem like a valid justification for ip6gre. > This fix sets 'addr_len' to the appropriate value, ensuring > consistency in the net_device setup for IPv6 GRE tunnels. > > Bug: Setting addr_len to the size of an IPv6 network address (16 bytes) > can cause a packet socket with SOCK_DGRAM to fail on 'sendto' calls. > This happens due to a check in 'packet_snd' for SOCK_DGRAM types, > which validates the address length. > > This bug was introduced in kernel version 4.20.0 and is still present in the current version. > > Steps to reproduce: > > ip -6 tunnel add <dev_name> mode ip6gre remote <remote_addr> local <local_addr> ttl 255 > ip link set dev <dev_name> up > busybox udhcpc -i <dev_name> -n -f > -> It returns Invalid Argument. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=202147 > Reported-by: Friedrich Oslage <friedrich@oslage.de> > Signed-off-by: Emanuele Santini <emanuele.santini.88@gmail.com> > --- > net/ipv6/ip6_gre.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > index 235808cfec70..db7679b04a02 100644 > --- a/net/ipv6/ip6_gre.c > +++ b/net/ipv6/ip6_gre.c > @@ -1455,7 +1455,7 @@ static void ip6gre_tunnel_setup(struct net_device *dev) > dev->type = ARPHRD_IP6GRE; > > dev->flags |= IFF_NOARP; > - dev->addr_len = sizeof(struct in6_addr); > + dev->addr_len = ETH_ALEN; I guess it should be "dev->addr_len = 0" instead. We have no "hardware" address. > netif_keep_dst(dev); > /* This perm addr will be used as interface identifier by IPv6 */ > dev->addr_assign_type = NET_ADDR_RANDOM; > -- > 2.46.0 > >
I'm talking about the ip6gre. I agree that setting the hardware address to 0 is appropriate.
However, in the ip6gre_tunnel_setup function, the perm_addr field of net_device is
currently assigned a random Ethernet address:
dev->flags |= IFF_NOARP;
- dev->addr_len = sizeof(struct in6_addr);
+ dev->addr_len = ETH_ALEN;
netif_keep_dst(dev);
/* This perm addr will be used as interface identifier by IPv6 */
dev->addr_assign_type = NET_ADDR_RANDOM;
eth_random_addr(dev->perm_addr);
maybe this is not a valid justification to set addr_len to ETH_ALEN.
I will make a review setting addr_len to 0, and will resubmit the patch after successful testing.
On Fri, Nov 08, 2024 at 03:24:03PM +0100, Emanuele Santini wrote: > I'm talking about the ip6gre. I agree that setting the hardware address to 0 is appropriate. > However, in the ip6gre_tunnel_setup function, the perm_addr field of net_device is > currently assigned a random Ethernet address: > > dev->flags |= IFF_NOARP; > - dev->addr_len = sizeof(struct in6_addr); > + dev->addr_len = ETH_ALEN; > netif_keep_dst(dev); > /* This perm addr will be used as interface identifier by IPv6 */ > dev->addr_assign_type = NET_ADDR_RANDOM; > eth_random_addr(dev->perm_addr); > > maybe this is not a valid justification to set addr_len to ETH_ALEN. I think that having a fake permanent address for the purpose of IPv6 interface Id. generation isn't a correct justification for setting dev->addr_len. If setting ->perm_addr and ->addr_assign_type have side effects on the acceptable values of ->addr_len, then the commit description should explain that in more details. > I will make a review setting addr_len to 0, and will resubmit the patch after successful testing. Thanks.
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index 235808cfec70..db7679b04a02 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -1455,7 +1455,7 @@ static void ip6gre_tunnel_setup(struct net_device *dev) dev->type = ARPHRD_IP6GRE; dev->flags |= IFF_NOARP; - dev->addr_len = sizeof(struct in6_addr); + dev->addr_len = ETH_ALEN; netif_keep_dst(dev); /* This perm addr will be used as interface identifier by IPv6 */ dev->addr_assign_type = NET_ADDR_RANDOM;
The 'addr_len' field in 'net_device' should represent the size of the hardware address for the device. While GRE tunneling does not require a hardware address, a random Ethernet address is still assigned to the 'net_device'. Therefore, the correct 'addr_len' value should be the size of an Ethernet address (6 bytes), not the size of an IPv6 address. This fix sets 'addr_len' to the appropriate value, ensuring consistency in the net_device setup for IPv6 GRE tunnels. Bug: Setting addr_len to the size of an IPv6 network address (16 bytes) can cause a packet socket with SOCK_DGRAM to fail on 'sendto' calls. This happens due to a check in 'packet_snd' for SOCK_DGRAM types, which validates the address length. This bug was introduced in kernel version 4.20.0 and is still present in the current version. Steps to reproduce: ip -6 tunnel add <dev_name> mode ip6gre remote <remote_addr> local <local_addr> ttl 255 ip link set dev <dev_name> up busybox udhcpc -i <dev_name> -n -f -> It returns Invalid Argument. Link: https://bugzilla.kernel.org/show_bug.cgi?id=202147 Reported-by: Friedrich Oslage <friedrich@oslage.de> Signed-off-by: Emanuele Santini <emanuele.santini.88@gmail.com> --- net/ipv6/ip6_gre.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)