net: gro: fix misuse of CB in udp socket lookup

PATCH Superseded Richard Gobert 5 participants 10 messages Jul 7, 2023 – Jul 19, 2023

Subsystems (3)

Networking Network drivers UDP

lore

Share & tools

lore

Richard Gobert

author

2023-07-07 12:17 UTC #0 lore raw

cover net: gro: fix misuse of CB in udp socket lookup

GRO stack uses udp_lib_lookup_skb which relies on IP/IPv6 CB's info, and at the GRO stage, CB holds napi_gro_cb info. Specifically, udp_lib_lookup_skb tries to fetch iff and flags information from the CB, to find the relevant udp tunnel socket (GENEVE/VXLAN/..). Up until a patch I submitted recently [0], it worked merely by luck, due to the layouts of napi_gro_cb and IP6CB.

AFAIU it worked because:
IP6CB(skb)->flags is at offset 16 inside IP6CB:
- Before the patch: flags was mapped to flush.
- After the patch: flags was mapped to data_offset.

IP6CB(skb)->iff is at offset 0 inside IP6CB:
- Before the patch: iif was mapped to frag0.
- After the patch: iif was mapped to a union of frag0 and last.

After my patch, on the receive phase, while data_offset is 40 (since IPv6 header is 40 bytes), inet_iif calls ipv6_l3mdev_skb, which checks whether IP6CB(skb)->flags's IP6SKB_L3SLAVE bit is on or off (in our case its off). If it is off, inet_iif returns IP6CB(skb)->iif, which is mapped to napi_gro_cb->frag0, making inet_iif return 0 most of the times. inet_sdif returns zero due to a similar reason caused by data_offset being equal to 40 (and less than 64).

On the other hand, the complete phase behaves differently. data_offset is usually greater than 64 and less than 128 so the IP6SKB_L3SLAVE flag is on. Thus, inet_sdif returns IP6CB(skb)->iif, which is mapped to last which contains a pointer. This causes udp_sk_bound_dev_eq to fail, which leads to udp6_lib_lookup2 failing and not returning a socket. This leads the receive phase of GRO to find the right socket, and on the complete phase, it fails to find it and makes the throughput go down to nearly zero.

Before [0] flags was mapped to flush. flush's possible values were 1 and 0, making inet6_iff always returning skb->skb_iif and inet6_sdif returning 0, and leading to udp_sk_bound_dev_eq returning true.

A fix is to not rely on CB, and get iff and sdif using skb->dev. l3mdev case requires special attention since it has a master and a slave device.

[0] https://lore.kernel.org/netdev/20230601160924.GA9194@debian/

Richard Gobert (1):
net: gro: fix misuse of CB in udp socket lookup

 include/net/udp.h      |  2 ++
 net/ipv4/udp.c         | 21 +++++++++++++++++++--
 net/ipv4/udp_offload.c |  7 +++++--
 net/ipv6/udp.c         | 21 +++++++++++++++++++--
 net/ipv6/udp_offload.c |  7 +++++--
 5 files changed, 50 insertions(+), 8 deletions(-)

--
2.36.1

Richard Gobert

author

2023-07-07 12:26 UTC #1 lore raw

PATCH 1/1 net: gro: fix misuse of CB in udp socket lookup

This patch fixes a misuse of IP{6}CB(skb) in GRO, while calling to udp6_lib_lookup2 when handling udp tunnels. udp6_lib_lookup2 fetch the device from CB. The fix changes it to fetch the device from skb->dev. l3mdev case requires special attention since it has a master and a slave device.

Fixes a6024562ffd7 ("udp: Add GRO functions to UDP socket")

Reported-by Gal Pressman <[email protected]>

Signed-off-by Richard Gobert <[email protected]>

---
 include/net/udp.h      |  2 ++
 net/ipv4/udp.c         | 21 +++++++++++++++++++--
 net/ipv4/udp_offload.c |  7 +++++--
 net/ipv6/udp.c         | 21 +++++++++++++++++++--
 net/ipv6/udp_offload.c |  7 +++++--
 5 files changed, 50 insertions(+), 8 deletions(-)

include/net/udp.h +2 -0

index 4d13424f8f72..48af1479882f 100644
@@ -299,6 +299,7 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
 int udp_lib_setsockopt(struct sock *sk, int level, int optname,
 		       sockptr_t optval, unsigned int optlen,
 		       int (*push_pending_frames)(struct sock *));
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
 			     __be32 daddr, __be16 dport, int dif);
 struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,

@@ -310,6 +311,7 @@ struct sock *udp6_lib_lookup(struct net *net,
 			     const struct in6_addr *saddr, __be16 sport,
 			     const struct in6_addr *daddr, __be16 dport,
 			     int dif);
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *__udp6_lib_lookup(struct net *net,
 			       const struct in6_addr *saddr, __be16 sport,
 			       const struct in6_addr *daddr, __be16 dport,

net/ipv4/udp.c +19 -2

index 42a96b3547c9..0581ab184afd 100644
@@ -550,15 +550,32 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
 				 inet_sdif(skb), udptable, skb);
 }
 
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+	*iif = inet_iif(skb) || skb->dev->ifindex;
+	*sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+	if (netif_is_l3_slave(skb->dev)) {
+		struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);
+		*sdif = *iif;
+		*iif = master ? master->ifindex : 0;
+	}
+#endif
+}
+
 struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
 				 __be16 sport, __be16 dport)
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp4_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp4_lib_lookup(net, iph->saddr, sport,
-				 iph->daddr, dport, inet_iif(skb),
-				 inet_sdif(skb), net->ipv4.udp_table, NULL);
+				 iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 /* Must be called under rcu_read_lock().

net/ipv4/udp_offload.c +5 -2

index 75aa4de5b731..70d760b271db 100644
@@ -603,10 +603,13 @@ static struct sock *udp4_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp4_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp4_lib_lookup(net, iph->saddr, sport,
-				 iph->daddr, dport, inet_iif(skb),
-				 inet_sdif(skb), net->ipv4.udp_table, NULL);
+				 iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 INDIRECT_CALLABLE_SCOPE

net/ipv6/udp.c +19 -2

index 317b01c9bc39..aac9b20d67e4 100644
@@ -294,15 +294,32 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
 				 inet6_sdif(skb), udptable, skb);
 }
 
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+	*iif = skb->dev->ifindex;
+	*sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+	if (netif_is_l3_slave(skb->dev)) {
+		struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);
+		*sdif = *iif;
+		*iif = master ? master->ifindex : 0;
+	}
+#endif
+}
+
 struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
 				 __be16 sport, __be16 dport)
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp6_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp6_lib_lookup(net, &iph->saddr, sport,
-				 &iph->daddr, dport, inet6_iif(skb),
-				 inet6_sdif(skb), net->ipv4.udp_table, NULL);
+				 &iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 /* Must be called under rcu_read_lock().

net/ipv6/udp_offload.c +5 -2

index ad3b8726873e..88191d79002e 100644
@@ -119,10 +119,13 @@ static struct sock *udp6_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp6_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp6_lib_lookup(net, &iph->saddr, sport,
-				 &iph->daddr, dport, inet6_iif(skb),
-				 inet6_sdif(skb), net->ipv4.udp_table, NULL);
+				 &iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 INDIRECT_CALLABLE_SCOPE

--
2.36.1

Eric Dumazet

2023-07-07 13:45 UTC #2 lore raw

Quoted 43 lines · Richard Gobert

Fixes a6024562ffd7 ("udp: Add GRO functions to UDP socket")

Reported-by Gal Pressman <[email protected]>

Signed-off-by Richard Gobert <[email protected]>

---
 include/net/udp.h      |  2 ++
 net/ipv4/udp.c         | 21 +++++++++++++++++++--
 net/ipv4/udp_offload.c |  7 +++++--
 net/ipv6/udp.c         | 21 +++++++++++++++++++--
 net/ipv6/udp_offload.c |  7 +++++--
 5 files changed, 50 insertions(+), 8 deletions(-)

include/net/udp.h +2 -0

index 4d13424f8f72..48af1479882f 100644
@@ -299,6 +299,7 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
 int udp_lib_setsockopt(struct sock *sk, int level, int optname,
                       sockptr_t optval, unsigned int optlen,
                       int (*push_pending_frames)(struct sock *));
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
                             __be32 daddr, __be16 dport, int dif);
 struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,

@@ -310,6 +311,7 @@ struct sock *udp6_lib_lookup(struct net *net,
                             const struct in6_addr *saddr, __be16 sport,
                             const struct in6_addr *daddr, __be16 dport,
                             int dif);
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *__udp6_lib_lookup(struct net *net,
                               const struct in6_addr *saddr, __be16 sport,
                               const struct in6_addr *daddr, __be16 dport,

net/ipv4/udp.c +0 -0

index 42a96b3547c9..0581ab184afd 100644

net/ipv4/udp.c static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb, · line 555

Quoted 6 lines

net/ipv4/udp.c +3 -0

@@ -550,15 +550,32 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
                                 inet_sdif(skb), udptable, skb);
 }
 
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+       *iif = inet_iif(skb) || skb->dev->ifindex;

This looks wrong. Did you mean

        *iif = inet_iif(skb) ?: skb->dev->ifindex;

Quoted 53 lines

net/ipv4/udp.c +16 -2

+       *sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+       if (netif_is_l3_slave(skb->dev)) {
+               struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);
+               *sdif = *iif;
+               *iif = master ? master->ifindex : 0;
+       }
+#endif
+}
+
 struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
                                 __be16 sport, __be16 dport)
 {
        const struct iphdr *iph = ip_hdr(skb);
        struct net *net = dev_net(skb->dev);
+       int iif, sdif;
+
+       udp4_get_iif_sdif(skb, &iif, &sdif);
 
        return __udp4_lib_lookup(net, iph->saddr, sport,
-                                iph->daddr, dport, inet_iif(skb),
-                                inet_sdif(skb), net->ipv4.udp_table, NULL);
+                                iph->daddr, dport, iif,
+                                sdif, net->ipv4.udp_table, NULL);
 }
 
 /* Must be called under rcu_read_lock().

net/ipv4/udp_offload.c +5 -2

index 75aa4de5b731..70d760b271db 100644
@@ -603,10 +603,13 @@ static struct sock *udp4_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
 {
        const struct iphdr *iph = skb_gro_network_header(skb);
        struct net *net = dev_net(skb->dev);
+       int iif, sdif;
+
+       udp4_get_iif_sdif(skb, &iif, &sdif);
 
        return __udp4_lib_lookup(net, iph->saddr, sport,
-                                iph->daddr, dport, inet_iif(skb),
-                                inet_sdif(skb), net->ipv4.udp_table, NULL);
+                                iph->daddr, dport, iif,
+                                sdif, net->ipv4.udp_table, NULL);
 }
 
 INDIRECT_CALLABLE_SCOPE

net/ipv6/udp.c +0 -0

index 317b01c9bc39..aac9b20d67e4 100644

net/ipv6/udp.c static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb, · line 299

Quoted 6 lines

net/ipv6/udp.c +3 -0

@@ -294,15 +294,32 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
                                 inet6_sdif(skb), udptable, skb);
 }
 
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+       *iif = skb->dev->ifindex;

ipv6_rcv() inits

IP6CB(skb)->iif = skb_dst(skb) ? ip6_dst_idev(skb_dst(skb))->dev->ifindex : dev->ifindex;

You chose to always use skb->dev->ifindex instead ?

You might add a comment why it is okay.

Quoted 51 lines

net/ipv6/udp.c +16 -2

+       *sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+       if (netif_is_l3_slave(skb->dev)) {
+               struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);
+               *sdif = *iif;
+               *iif = master ? master->ifindex : 0;
+       }
+#endif
+}
+
 struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
                                 __be16 sport, __be16 dport)
 {
        const struct ipv6hdr *iph = ipv6_hdr(skb);
        struct net *net = dev_net(skb->dev);
+       int iif, sdif;
+
+       udp6_get_iif_sdif(skb, &iif, &sdif);
 
        return __udp6_lib_lookup(net, &iph->saddr, sport,
-                                &iph->daddr, dport, inet6_iif(skb),
-                                inet6_sdif(skb), net->ipv4.udp_table, NULL);
+                                &iph->daddr, dport, iif,
+                                sdif, net->ipv4.udp_table, NULL);
 }
 
 /* Must be called under rcu_read_lock().

net/ipv6/udp_offload.c +5 -3

index ad3b8726873e..88191d79002e 100644
@@ -119,10 +119,13 @@ static struct sock *udp6_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
 {
        const struct ipv6hdr *iph = skb_gro_network_header(skb);
        struct net *net = dev_net(skb->dev);
+       int iif, sdif;
+
+       udp6_get_iif_sdif(skb, &iif, &sdif);
 
        return __udp6_lib_lookup(net, &iph->saddr, sport,
-                                &iph->daddr, dport, inet6_iif(skb),
-                                inet6_sdif(skb), net->ipv4.udp_table, NULL);
+                                &iph->daddr, dport, iif,
+                                sdif, net->ipv4.udp_table, NULL);
 }
 
 INDIRECT_CALLABLE_SCOPE
--

2.36.1

Richard Gobert

author

2023-07-10 14:54 UTC #3 lore raw

Thanks, I will send a v2 with the fixes.

Quoted 8 lines

ipv6_rcv() inits
IP6CB(skb)->iif = skb_dst(skb) ? ip6_dst_idev(skb_dst(skb))->dev->ifindex : dev->ifindex;
You chose to always use skb->dev->ifindex instead ?
You might add a comment why it is okay.

It looks like this assignment ("ip6_dst_idev(skb_dst(skb))->dev->ifindex") is relevant only in case of sending traffic on loopback.
It does not seem relevant in GRO stack.
That is why I chose to use only the skb->dev->ifindex part. Do you think that's correct?

Kuniyuki Iwashima

2023-07-07 18:42 UTC #4 lore raw

In reply to Richard Gobert

From: Richard Gobert <[email protected]>
Date: Fri, 7 Jul 2023 14:26:28 +0200

Quoted 43 lines

Fixes a6024562ffd7 ("udp: Add GRO functions to UDP socket")

Reported-by Gal Pressman <[email protected]>

Signed-off-by Richard Gobert <[email protected]>

---
 include/net/udp.h      |  2 ++
 net/ipv4/udp.c         | 21 +++++++++++++++++++--
 net/ipv4/udp_offload.c |  7 +++++--
 net/ipv6/udp.c         | 21 +++++++++++++++++++--
 net/ipv6/udp_offload.c |  7 +++++--
 5 files changed, 50 insertions(+), 8 deletions(-)

include/net/udp.h +2 -0

index 4d13424f8f72..48af1479882f 100644
@@ -299,6 +299,7 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
 int udp_lib_setsockopt(struct sock *sk, int level, int optname,
 		       sockptr_t optval, unsigned int optlen,
 		       int (*push_pending_frames)(struct sock *));
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
 			     __be32 daddr, __be16 dport, int dif);
 struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,

@@ -310,6 +311,7 @@ struct sock *udp6_lib_lookup(struct net *net,
 			     const struct in6_addr *saddr, __be16 sport,
 			     const struct in6_addr *daddr, __be16 dport,
 			     int dif);
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *__udp6_lib_lookup(struct net *net,
 			       const struct in6_addr *saddr, __be16 sport,
 			       const struct in6_addr *daddr, __be16 dport,

net/ipv4/udp.c +0 -0

index 42a96b3547c9..0581ab184afd 100644
@@ -550,15 +550,32 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
 				 inet_sdif(skb), udptable, skb);

net/ipv4/udp.c static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb, · line 560

Quoted 10 lines

net/ipv4/udp.c +8 -0

 }
 
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+	*iif = inet_iif(skb) || skb->dev->ifindex;
+	*sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+	if (netif_is_l3_slave(skb->dev)) {
+		struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);

nit: blank line here for checkpatch.

Quoted 49 lines

net/ipv4/udp.c +11 -2

+		*sdif = *iif;
+		*iif = master ? master->ifindex : 0;
+	}
+#endif
+}
+
 struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
 				 __be16 sport, __be16 dport)
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp4_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp4_lib_lookup(net, iph->saddr, sport,
-				 iph->daddr, dport, inet_iif(skb),
-				 inet_sdif(skb), net->ipv4.udp_table, NULL);
+				 iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 /* Must be called under rcu_read_lock().

net/ipv4/udp_offload.c +5 -2

index 75aa4de5b731..70d760b271db 100644
@@ -603,10 +603,13 @@ static struct sock *udp4_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp4_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp4_lib_lookup(net, iph->saddr, sport,
-				 iph->daddr, dport, inet_iif(skb),
-				 inet_sdif(skb), net->ipv4.udp_table, NULL);
+				 iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 INDIRECT_CALLABLE_SCOPE

net/ipv6/udp.c +0 -0

index 317b01c9bc39..aac9b20d67e4 100644
@@ -294,15 +294,32 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
 				 inet6_sdif(skb), udptable, skb);

net/ipv6/udp.c static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb, · line 304

Quoted 10 lines

net/ipv6/udp.c +8 -0

 }
 
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+	*iif = skb->dev->ifindex;
+	*sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+	if (netif_is_l3_slave(skb->dev)) {
+		struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);

Same here.

Quoted 45 lines

net/ipv6/udp.c +11 -2

+		*sdif = *iif;
+		*iif = master ? master->ifindex : 0;
+	}
+#endif
+}
+
 struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
 				 __be16 sport, __be16 dport)
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp6_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp6_lib_lookup(net, &iph->saddr, sport,
-				 &iph->daddr, dport, inet6_iif(skb),
-				 inet6_sdif(skb), net->ipv4.udp_table, NULL);
+				 &iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 /* Must be called under rcu_read_lock().

net/ipv6/udp_offload.c +5 -3

index ad3b8726873e..88191d79002e 100644
@@ -119,10 +119,13 @@ static struct sock *udp6_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
 	struct net *net = dev_net(skb->dev);
+	int iif, sdif;
+
+	udp6_get_iif_sdif(skb, &iif, &sdif);
 
 	return __udp6_lib_lookup(net, &iph->saddr, sport,
-				 &iph->daddr, dport, inet6_iif(skb),
-				 inet6_sdif(skb), net->ipv4.udp_table, NULL);
+				 &iph->daddr, dport, iif,
+				 sdif, net->ipv4.udp_table, NULL);
 }
 
 INDIRECT_CALLABLE_SCOPE
--

2.36.1

David Ahern

2023-07-07 21:40 UTC #5 lore raw

In reply to Richard Gobert

Quoted 5 lines · Richard Gobert

This patch fixes a misuse of IP{6}CB(skb) in GRO, while calling to udp6_lib_lookup2 when handling udp tunnels. udp6_lib_lookup2 fetch the device from CB. The fix changes it to fetch the device from skb->dev. l3mdev case requires special attention since it has a master and a slave device.

put your cover letter details in here; no need for a cover letter for a single patch.

Quoted 43 lines

Fixes a6024562ffd7 ("udp: Add GRO functions to UDP socket")

Reported-by Gal Pressman <[email protected]>

Signed-off-by Richard Gobert <[email protected]>

---
 include/net/udp.h      |  2 ++
 net/ipv4/udp.c         | 21 +++++++++++++++++++--
 net/ipv4/udp_offload.c |  7 +++++--
 net/ipv6/udp.c         | 21 +++++++++++++++++++--
 net/ipv6/udp_offload.c |  7 +++++--
 5 files changed, 50 insertions(+), 8 deletions(-)

include/net/udp.h +2 -0

index 4d13424f8f72..48af1479882f 100644
@@ -299,6 +299,7 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
 int udp_lib_setsockopt(struct sock *sk, int level, int optname,
 		       sockptr_t optval, unsigned int optlen,
 		       int (*push_pending_frames)(struct sock *));
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
 			     __be32 daddr, __be16 dport, int dif);
 struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,

@@ -310,6 +311,7 @@ struct sock *udp6_lib_lookup(struct net *net,
 			     const struct in6_addr *saddr, __be16 sport,
 			     const struct in6_addr *daddr, __be16 dport,
 			     int dif);
+void udp6_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif);
 struct sock *__udp6_lib_lookup(struct net *net,
 			       const struct in6_addr *saddr, __be16 sport,
 			       const struct in6_addr *daddr, __be16 dport,

net/ipv4/udp.c +3 -0

index 42a96b3547c9..0581ab184afd 100644
@@ -550,15 +550,32 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
 				 inet_sdif(skb), udptable, skb);
 }
 
+void udp4_get_iif_sdif(const struct sk_buff *skb, int *iif, int *sdif)
+{
+	*iif = inet_iif(skb) || skb->dev->ifindex;

net/ipv4/udp.c static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb, · line 565

Quoted 10 lines

net/ipv4/udp.c +10 -0

+	*sdif = 0;
+
+#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
+	if (netif_is_l3_slave(skb->dev)) {
+		struct net_device *master = netdev_master_upper_dev_get_rcu(skb->dev);
+		*sdif = *iif;
+		*iif = master ? master->ifindex : 0;
+	}
+#endif
+}

there are existing iif and sdif lookup functions. I believe this gro path needs a different version, but it should have a comment of when it can be used vs the existing ones. Also, it is small enough to be an inline like the existing ones. e.g., see inet_sdif

Richard Gobert

author

2023-07-10 14:58 UTC #6 lore raw

Quoted 2 lines

put your cover letter details in here; no need for a cover letter for a single patch.

I believe some details are irrelevant to the bugfix itself, I prefer to avoid overloading the commit message... Do you think there is a specific part of the cover letter that should be added to the commit message?

Quoted 4 lines

there are existing iif and sdif lookup functions. I believe this gro path needs a different version, but it should have a comment of when it can be used vs the existing ones. Also, it is small enough to be an inline like the existing ones. e.g., see inet_sdif

I was under the impression the coding style of Linux does not encourage placing the inline keyword.
In which cases do you think I should add it?

David Ahern

2023-07-10 15:50 UTC #7 lore raw

Quoted 8 lines · Richard Gobert

Quoted 2 lines
put your cover letter details in here; no need for a cover letter for a single patch.
I believe some details are irrelevant to the bugfix itself, I prefer to avoid overloading the commit message... Do you think there is a specific part of the cover letter that should be added to the commit message?

Quoted 8 lines

Quoted 4 lines
there are existing iif and sdif lookup functions. I believe this gro path needs a different version, but it should have a comment of when it can be used vs the existing ones. Also, it is small enough to be an inline like the existing ones. e.g., see inet_sdif
I was under the impression the coding style of Linux does not encourage placing the inline keyword.
In which cases do you think I should add it?

See the existing *_sdif helpers in include/net/ip.h, include/linux/ipv6.h and include/net/tcp.h.

Gal Pressman

2023-07-17 10:42 UTC #8 lore raw

In reply to Richard Gobert

Quoted 9 lines · Richard Gobert

This patch fixes a misuse of IP{6}CB(skb) in GRO, while calling to udp6_lib_lookup2 when handling udp tunnels. udp6_lib_lookup2 fetch the device from CB. The fix changes it to fetch the device from skb->dev. l3mdev case requires special attention since it has a master and a slave device.
Fixes a6024562ffd7 ("udp: Add GRO functions to UDP socket")
Reported-by Gal Pressman <[email protected]>
Signed-off-by Richard Gobert <[email protected]>

Hi Richard,
Are you planning to submit a v2 for this patch?

Richard Gobert

author

2023-07-19 07:28 UTC #9 lore raw

Hi Gal,
Yes, I'm planning to implement all the fixes and submit a v2 in the next few days.