| Message ID | 156468243184.27559.7002090473019021952.stgit@firesoul |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: fix regressions for generic-XDP | expand |
On Thu, 01 Aug 2019 20:00:31 +0200, Jesper Dangaard Brouer wrote: > When generic-XDP was moved to a later processing step by commit > 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") > a regression was introduced when using bpf_xdp_adjust_head. > > The issue is that after this commit the skb->network_header is now > changed prior to calling generic XDP and not after. Thus, if the header > is changed by XDP (via bpf_xdp_adjust_head), then skb->network_header > also need to be updated again. Fix by calling skb_reset_network_header(). > > Fixes: 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") > Reported-by: Brandon Cazander <brandon.cazander@multapplied.net> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> Out of curiosity what was your conclusion regarding resetting the transport header as well?
On Thu, 1 Aug 2019 17:44:06 -0700 Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > On Thu, 01 Aug 2019 20:00:31 +0200, Jesper Dangaard Brouer wrote: > > When generic-XDP was moved to a later processing step by commit > > 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") > > a regression was introduced when using bpf_xdp_adjust_head. > > > > The issue is that after this commit the skb->network_header is now > > changed prior to calling generic XDP and not after. Thus, if the header > > is changed by XDP (via bpf_xdp_adjust_head), then skb->network_header > > also need to be updated again. Fix by calling skb_reset_network_header(). > > > > Fixes: 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") > > Reported-by: Brandon Cazander <brandon.cazander@multapplied.net> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > Out of curiosity what was your conclusion regarding resetting the > transport header as well? Well, I don't know... I need some review, from e.g. Stephen that changed this... I've added code snippets below signature to helper reviewers (also helps understand below paragraph). I think, we perhaps should call skb_reset_transport_header(), as we change skb->data (via either __skb_pull() or __skb_push()), *BUT* I'm not sure it is needed/required, as someone/something afterwards still need to call skb_set_transport_header(), which also calls skb_reset_transport_header() anyway. I also want to ask reviewers, if we should move the call to skb_reset_mac_len() (which Stephen added) in __netif_receive_skb_core() into netif_receive_generic_xdp() (after the call to skb_reset_network_header()), (it would be more natural to keep them together)? - - Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer Code snippet /* check if bpf_xdp_adjust_head was used */ off = xdp->data - orig_data; if (off) { if (off > 0) __skb_pull(skb, off); else if (off < 0) __skb_push(skb, -off); skb->mac_header += off; skb_reset_network_header(skb); } static inline bool skb_transport_header_was_set(const struct sk_buff *skb) { return skb->transport_header != (typeof(skb->transport_header))~0U; } static inline unsigned char *skb_transport_header(const struct sk_buff *skb) { return skb->head + skb->transport_header; } static inline void skb_reset_transport_header(struct sk_buff *skb) { skb->transport_header = skb->data - skb->head; } static inline void skb_set_transport_header(struct sk_buff *skb, const int offset) { skb_reset_transport_header(skb); skb->transport_header += offset; }
On Fri, 2 Aug 2019 09:53:54 +0200, Jesper Dangaard Brouer wrote: > On Thu, 1 Aug 2019 17:44:06 -0700 > Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > > On Thu, 01 Aug 2019 20:00:31 +0200, Jesper Dangaard Brouer wrote: > > > When generic-XDP was moved to a later processing step by commit > > > 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") > > > a regression was introduced when using bpf_xdp_adjust_head. > > > > > > The issue is that after this commit the skb->network_header is now > > > changed prior to calling generic XDP and not after. Thus, if the header > > > is changed by XDP (via bpf_xdp_adjust_head), then skb->network_header > > > also need to be updated again. Fix by calling skb_reset_network_header(). > > > > > > Fixes: 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") > > > Reported-by: Brandon Cazander <brandon.cazander@multapplied.net> > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > > > Out of curiosity what was your conclusion regarding resetting the > > transport header as well? > > Well, I don't know... I need some review, from e.g. Stephen that > changed this... I've added code snippets below signature to helper > reviewers (also helps understand below paragraph). > > I think, we perhaps should call skb_reset_transport_header(), as we > change skb->data (via either __skb_pull() or __skb_push()), *BUT* I'm > not sure it is needed/required, as someone/something afterwards still > need to call skb_set_transport_header(), which also calls > skb_reset_transport_header() anyway. Perhaps you've seen this, but just in case - this is the last commit that touched the transport header setting in __netif_receive_skb(), and it sounds like it matters mostly for qdisc accounting? commit fda55eca5a33f33ffcd4192c6b2d75179714a52c Author: Eric Dumazet <edumazet@google.com> Date: Mon Jan 7 09:28:21 2013 +0000 net: introduce skb_transport_header_was_set() We have skb_mac_header_was_set() helper to tell if mac_header was set on a skb. We would like the same for transport_header. __netif_receive_skb() doesn't reset the transport header if already set by GRO layer. Note that network stacks usually reset the transport header anyway, after pulling the network header, so this change only allows a followup patch to have more precise qdisc pkt_len computation for GSO packets at ingress side. Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/net/core/dev.c b/net/core/dev.c index fc676b2610e3..b5533795c3c1 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4374,12 +4374,17 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb, act = bpf_prog_run_xdp(xdp_prog, xdp); + /* check if bpf_xdp_adjust_head was used */ off = xdp->data - orig_data; - if (off > 0) - __skb_pull(skb, off); - else if (off < 0) - __skb_push(skb, -off); - skb->mac_header += off; + if (off) { + if (off > 0) + __skb_pull(skb, off); + else if (off < 0) + __skb_push(skb, -off); + + skb->mac_header += off; + skb_reset_network_header(skb); + } /* check if bpf_xdp_adjust_tail was used. it can only "shrink" * pckt.
When generic-XDP was moved to a later processing step by commit 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") a regression was introduced when using bpf_xdp_adjust_head. The issue is that after this commit the skb->network_header is now changed prior to calling generic XDP and not after. Thus, if the header is changed by XDP (via bpf_xdp_adjust_head), then skb->network_header also need to be updated again. Fix by calling skb_reset_network_header(). Fixes: 458bf2f224f0 ("net: core: support XDP generic on stacked devices.") Reported-by: Brandon Cazander <brandon.cazander@multapplied.net> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- net/core/dev.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)