| Message ID | 1610110348-119768-1-git-send-email-dseok.yi@samsung.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] udp: check sk for UDP GRO fraglist | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 50 this patch: 50 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 17 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 42 this patch: 42 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Fri, Jan 08, 2021 at 09:52:28PM +0900, Dongseok Yi wrote: > It is a workaround patch. > > UDP/IP header of UDP GROed frag_skbs are not updated even after NAT > forwarding. Only the header of head_skb from ip_finish_output_gso -> > skb_gso_segment is updated but following frag_skbs are not updated. > > A call path skb_mac_gso_segment -> inet_gso_segment -> > udp4_ufo_fragment -> __udp_gso_segment -> __udp_gso_segment_list > does not try to update any UDP/IP header of the segment list. > > It might make sense because each skb of frag_skbs is converted to a > list of regular packets. Header update with checksum calculation may > be not needed for UDP GROed frag_skbs. > > But UDP GRO frag_list is started from udp_gro_receive, we don't know > whether the skb will be NAT forwarded at that time. For workaround, > try to get sock always when call udp4_gro_receive -> udp_gro_receive > to check if the skb is for local. > > I'm still not sure if UDP GRO frag_list is really designed for local > session only. Can kernel support NAT forward for UDP GRO frag_list? > What am I missing? The initial idea when I implemented this was to have a fast forwarding path for UDP. So forwarding is a usecase, but NAT is a problem, indeed. A quick fix could be to segment the skb before it gets NAT forwarded. Alternatively we could check for a header change in __udp_gso_segment_list and update the header of the frag_skbs accordingly in that case.
On 2021-01-08 22:35, Steffen Klassert wrote: > On Fri, Jan 08, 2021 at 09:52:28PM +0900, Dongseok Yi wrote: > > It is a workaround patch. > > > > UDP/IP header of UDP GROed frag_skbs are not updated even after NAT > > forwarding. Only the header of head_skb from ip_finish_output_gso -> > > skb_gso_segment is updated but following frag_skbs are not updated. > > > > A call path skb_mac_gso_segment -> inet_gso_segment -> > > udp4_ufo_fragment -> __udp_gso_segment -> __udp_gso_segment_list > > does not try to update any UDP/IP header of the segment list. > > > > It might make sense because each skb of frag_skbs is converted to a > > list of regular packets. Header update with checksum calculation may > > be not needed for UDP GROed frag_skbs. > > > > But UDP GRO frag_list is started from udp_gro_receive, we don't know > > whether the skb will be NAT forwarded at that time. For workaround, > > try to get sock always when call udp4_gro_receive -> udp_gro_receive > > to check if the skb is for local. > > > > I'm still not sure if UDP GRO frag_list is really designed for local > > session only. Can kernel support NAT forward for UDP GRO frag_list? > > What am I missing? > > The initial idea when I implemented this was to have a fast > forwarding path for UDP. So forwarding is a usecase, but NAT > is a problem, indeed. A quick fix could be to segment the > skb before it gets NAT forwarded. Alternatively we could > check for a header change in __udp_gso_segment_list and > update the header of the frag_skbs accordingly in that case. Thank you for explaining. Can I think of it as a known issue? I think we should have a fix because NAT can be triggered by user. Can I check the current status? Already planning a patch or a new patch should be written?
On Mon, Jan 11, 2021 at 11:02:42AM +0900, Dongseok Yi wrote: > On 2021-01-08 22:35, Steffen Klassert wrote: > > On Fri, Jan 08, 2021 at 09:52:28PM +0900, Dongseok Yi wrote: > > > It is a workaround patch. > > > > > > UDP/IP header of UDP GROed frag_skbs are not updated even after NAT > > > forwarding. Only the header of head_skb from ip_finish_output_gso -> > > > skb_gso_segment is updated but following frag_skbs are not updated. > > > > > > A call path skb_mac_gso_segment -> inet_gso_segment -> > > > udp4_ufo_fragment -> __udp_gso_segment -> __udp_gso_segment_list > > > does not try to update any UDP/IP header of the segment list. > > > > > > It might make sense because each skb of frag_skbs is converted to a > > > list of regular packets. Header update with checksum calculation may > > > be not needed for UDP GROed frag_skbs. > > > > > > But UDP GRO frag_list is started from udp_gro_receive, we don't know > > > whether the skb will be NAT forwarded at that time. For workaround, > > > try to get sock always when call udp4_gro_receive -> udp_gro_receive > > > to check if the skb is for local. > > > > > > I'm still not sure if UDP GRO frag_list is really designed for local > > > session only. Can kernel support NAT forward for UDP GRO frag_list? > > > What am I missing? > > > > The initial idea when I implemented this was to have a fast > > forwarding path for UDP. So forwarding is a usecase, but NAT > > is a problem, indeed. A quick fix could be to segment the > > skb before it gets NAT forwarded. Alternatively we could > > check for a header change in __udp_gso_segment_list and > > update the header of the frag_skbs accordingly in that case. > > Thank you for explaining. > Can I think of it as a known issue? No, it was not known before you reported it. > I think we should have a fix > because NAT can be triggered by user. Can I check the current status? > Already planning a patch or a new patch should be written? We have to do a new patch to fix that issue. If you want do do so, go ahead.
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index ff39e94..d476216 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -457,7 +457,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb, int flush = 1; NAPI_GRO_CB(skb)->is_flist = 0; - if (skb->dev->features & NETIF_F_GRO_FRAGLIST) + if (sk && (skb->dev->features & NETIF_F_GRO_FRAGLIST)) NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1; if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) { @@ -537,8 +537,7 @@ struct sk_buff *udp4_gro_receive(struct list_head *head, struct sk_buff *skb) NAPI_GRO_CB(skb)->is_ipv6 = 0; rcu_read_lock(); - if (static_branch_unlikely(&udp_encap_needed_key)) - sk = udp4_gro_lookup_skb(skb, uh->source, uh->dest); + sk = udp4_gro_lookup_skb(skb, uh->source, uh->dest); pp = udp_gro_receive(head, skb, uh, sk); rcu_read_unlock();
It is a workaround patch. UDP/IP header of UDP GROed frag_skbs are not updated even after NAT forwarding. Only the header of head_skb from ip_finish_output_gso -> skb_gso_segment is updated but following frag_skbs are not updated. A call path skb_mac_gso_segment -> inet_gso_segment -> udp4_ufo_fragment -> __udp_gso_segment -> __udp_gso_segment_list does not try to update any UDP/IP header of the segment list. It might make sense because each skb of frag_skbs is converted to a list of regular packets. Header update with checksum calculation may be not needed for UDP GROed frag_skbs. But UDP GRO frag_list is started from udp_gro_receive, we don't know whether the skb will be NAT forwarded at that time. For workaround, try to get sock always when call udp4_gro_receive -> udp_gro_receive to check if the skb is for local. I'm still not sure if UDP GRO frag_list is really designed for local session only. Can kernel support NAT forward for UDP GRO frag_list? What am I missing? Fixes: 9fd1ff5d2ac7 (udp: Support UDP fraglist GRO/GSO.) Signed-off-by: Dongseok Yi <dseok.yi@samsung.com> --- net/ipv4/udp_offload.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)