net: gro: fix misuse of CB in udp socket lookup
Status from this message: This patch was applied to netdev/net.git (main)
Get series with b4
b4 am 20230727152503.GA32010@debianVersions newest first · linked by author and title
- v3 Latest1 patch 1 replyApplied 1/1
- v21 patch 3 repliesSuperseded
- v11 patch 8 repliesSuperseded
Changes in v3 from the cover letter
* change functions to static inline in the header file
Patches in v3 All 1 applied
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/1 | net: gro: fix misuse of CB in udp socket lookupinclude/ net/ net/ +2 more | +65 | 1 reply | Applied |
Reviews trailers per patch
The patch has a Reviewed-by or Acked-by.
| Patch | David Ahern |
|---|---|
| 1/1 | R-b |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [PATCH v3 0/1] 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/
Changelog:
v2 -> v3:
* change functions to static inline in the header file
v1 -> v2:
* make functions inline
* fix logical bug
* add a comment when we can use the new functions
* checkpatch fixes
Richard Gobert (1):
net: gro: fix misuse of CB in udp socket lookup
include/net/gro.h | 43 ++++++++++++++++++++++++++++++++++++++++++ net/ipv4/udp.c | 8 ++++++-- net/ipv4/udp_offload.c | 7 +++++-- net/ipv6/udp.c | 8 ++++++-- net/ipv6/udp_offload.c | 7 +++++-- 5 files changed, 65 insertions(+), 8 deletions(-)
Signature · 1 line
2.36.1