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
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(-)
diff --git a/include/net/udp.h b/include/net/udp.h
index 4d13424f8f72..48af1479882f 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -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,
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 42a96b3547c9..0581ab184afd 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -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().
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 75aa4de5b731..70d760b271db 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -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
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 317b01c9bc39..aac9b20d67e4 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -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().
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index ad3b8726873e..88191d79002e 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -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
On Fri, Jul 7, 2023 at 2:26 PM Richard Gobert <[email protected]> wrote:
>
> 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(-)
>
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 4d13424f8f72..48af1479882f 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -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,
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 42a96b3547c9..0581ab184afd 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -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;
> + *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().
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 75aa4de5b731..70d760b271db 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -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
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 317b01c9bc39..aac9b20d67e4 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -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.
> + *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().
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index ad3b8726873e..88191d79002e 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -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
>
From: Richard Gobert <[email protected]>
Date: Fri, 7 Jul 2023 14:26:28 +0200
> 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(-)
>
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 4d13424f8f72..48af1479882f 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -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,
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 42a96b3547c9..0581ab184afd 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -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);
nit: blank line here for checkpatch.
> + *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().
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 75aa4de5b731..70d760b271db 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -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
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 317b01c9bc39..aac9b20d67e4 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -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);
Same here.
> + *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().
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index ad3b8726873e..88191d79002e 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -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
On 7/7/23 6:26 AM, Richard Gobert wrote:
> 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.
>
> 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(-)
>
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 4d13424f8f72..48af1479882f 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -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,
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 42a96b3547c9..0581ab184afd 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -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
> +}
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
> 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?
> 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?
Thanks, I will send a v2 with the fixes.
> 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?
On 7/10/23 8:58 AM, Richard Gobert wrote:
>> 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?
>
>> 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.
On 07/07/2023 15:26, Richard Gobert wrote:
> 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?
Hi Gal,
Yes, I'm planning to implement all the fixes and submit a v2 in the next few days.