| Message ID | 20181121021309.6595-2-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,1/2] net: introduce skb_network_header_was_set() | expand |
On 11/20/2018 06:13 PM, Cong Wang wrote: > Currently, we only dump a few selected skb fields in > netdev_rx_csum_fault(). It is not suffient for debugging checksum > fault. This patch introduces skb_dump() which dumps skb mac header, > network header and its whole skb->data too. > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Eric Dumazet <edumazet@google.com> > Cc: David Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1, > + skb->data, skb->len, false); As I mentioned to David, we want all the bytes that were maybe already pulled (skb->head starting point, not skb->data) Also we will miss the trimmed bytes if there were padding data. And it seems the various bugs we have are all tied to the pulled or trimmed bytes. Thanks.
On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > Currently, we only dump a few selected skb fields in > > netdev_rx_csum_fault(). It is not suffient for debugging checksum > > fault. This patch introduces skb_dump() which dumps skb mac header, > > network header and its whole skb->data too. > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: David Miller <davem@davemloft.net> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > --- > > > > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1, > > + skb->data, skb->len, false); > > As I mentioned to David, we want all the bytes that were maybe already pulled > > (skb->head starting point, not skb->data) Hmm, with mac header and network header, it is effectively from skb->head, no? Is there anything between skb->head and mac header? > > Also we will miss the trimmed bytes if there were padding data. > And it seems the various bugs we have are all tied to the pulled or trimmed bytes. > Unless I miss something, the tailing padding data should be in range [iphdr->tot_len, skb->len]. No? Thanks
On Wed, Nov 21, 2018 at 10:17 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > Currently, we only dump a few selected skb fields in > > > netdev_rx_csum_fault(). It is not suffient for debugging checksum > > > fault. This patch introduces skb_dump() which dumps skb mac header, > > > network header and its whole skb->data too. > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > Cc: Eric Dumazet <edumazet@google.com> > > > Cc: David Miller <davem@davemloft.net> > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > --- > > > > > > > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1, > > > + skb->data, skb->len, false); > > > > As I mentioned to David, we want all the bytes that were maybe already pulled > > > > (skb->head starting point, not skb->data) > > Hmm, with mac header and network header, it is effectively from skb->head, no? > Is there anything between skb->head and mac header? Oh, I guess we wanted a single hex dump, or we need some user program to be able to rebuild from different memory zones the original CHECKSUM_COMPLETE value. > > > > > Also we will miss the trimmed bytes if there were padding data. > > And it seems the various bugs we have are all tied to the pulled or trimmed bytes. > > > > Unless I miss something, the tailing padding data should be in range > [iphdr->tot_len, skb->len]. No? Not after we did the pskb_trim_rcsum() call, since it has effectively reduced skb->len by the number of padding bytes.
On Wed, 2018-11-21 at 10:26 -0800, Eric Dumazet wrote: > On Wed, Nov 21, 2018 at 10:17 AM Cong Wang <xiyou.wangcong@gmail.com> > wrote: > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet < > > eric.dumazet@gmail.com> wrote: > > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > > Currently, we only dump a few selected skb fields in > > > > netdev_rx_csum_fault(). It is not suffient for debugging > > > > checksum > > > > fault. This patch introduces skb_dump() which dumps skb mac > > > > header, > > > > network header and its whole skb->data too. > > > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > Cc: Eric Dumazet <edumazet@google.com> > > > > Cc: David Miller <davem@davemloft.net> > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > --- > > > > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, > > > > 16, 1, > > > > + skb->data, skb->len, false); > > > > > > As I mentioned to David, we want all the bytes that were maybe > > > already pulled > > > > > > (skb->head starting point, not skb->data) > > > > Hmm, with mac header and network header, it is effectively from > > skb->head, no? > > Is there anything between skb->head and mac header? > > Oh, I guess we wanted a single hex dump, or we need some user program > to be able to > rebuild from different memory zones the original CHECKSUM_COMPLETE > value. > Normally the driver keeps some headroom @skb->head, so the actual mac header starts @ skb->head + driver_specific_headroom for example in mlx5 we do: va = page_addr + offset build_skb(va) /* skb->data = va + headroom */ skb_reserve(mlx5_headroom) > > > Also we will miss the trimmed bytes if there were padding data. > > > And it seems the various bugs we have are all tied to the pulled > > > or trimmed bytes. > > > > > > > Unless I miss something, the tailing padding data should be in > > range > > [iphdr->tot_len, skb->len]. No? > > Not after we did the pskb_trim_rcsum() call, since it has effectively > reduced skb->len by the number of padding bytes.
On Wed, Nov 21, 2018 at 10:26 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Nov 21, 2018 at 10:17 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > > Currently, we only dump a few selected skb fields in > > > > netdev_rx_csum_fault(). It is not suffient for debugging checksum > > > > fault. This patch introduces skb_dump() which dumps skb mac header, > > > > network header and its whole skb->data too. > > > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > Cc: Eric Dumazet <edumazet@google.com> > > > > Cc: David Miller <davem@davemloft.net> > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > --- > > > > > > > > > > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1, > > > > + skb->data, skb->len, false); > > > > > > As I mentioned to David, we want all the bytes that were maybe already pulled > > > > > > (skb->head starting point, not skb->data) > > > > Hmm, with mac header and network header, it is effectively from skb->head, no? > > Is there anything between skb->head and mac header? > > Oh, I guess we wanted a single hex dump, or we need some user program > to be able to > rebuild from different memory zones the original CHECKSUM_COMPLETE value. Yeah, I can remove the prefix and dump the complete packet as one single block. This means I also need to check where skb->data points to. > > > > > > > > > Also we will miss the trimmed bytes if there were padding data. > > > And it seems the various bugs we have are all tied to the pulled or trimmed bytes. > > > > > > > Unless I miss something, the tailing padding data should be in range > > [iphdr->tot_len, skb->len]. No? > > > Not after we did the pskb_trim_rcsum() call, since it has effectively > reduced skb->len by the number of padding bytes. Sure, this patch can't change where netdev_rx_csum_fault() gets called. We either need to move the checksum validation earlier, or move the trimming later, none of them belongs to this patch. Thanks.
On Wed, Nov 21, 2018 at 11:33 AM Saeed Mahameed <saeedm@mellanox.com> wrote: > > On Wed, 2018-11-21 at 10:26 -0800, Eric Dumazet wrote: > > On Wed, Nov 21, 2018 at 10:17 AM Cong Wang <xiyou.wangcong@gmail.com> > > wrote: > > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet < > > > eric.dumazet@gmail.com> wrote: > > > > > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > > > Currently, we only dump a few selected skb fields in > > > > > netdev_rx_csum_fault(). It is not suffient for debugging > > > > > checksum > > > > > fault. This patch introduces skb_dump() which dumps skb mac > > > > > header, > > > > > network header and its whole skb->data too. > > > > > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > > Cc: Eric Dumazet <edumazet@google.com> > > > > > Cc: David Miller <davem@davemloft.net> > > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > > --- > > > > > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, > > > > > 16, 1, > > > > > + skb->data, skb->len, false); > > > > > > > > As I mentioned to David, we want all the bytes that were maybe > > > > already pulled > > > > > > > > (skb->head starting point, not skb->data) > > > > > > Hmm, with mac header and network header, it is effectively from > > > skb->head, no? > > > Is there anything between skb->head and mac header? > > > > Oh, I guess we wanted a single hex dump, or we need some user program > > to be able to > > rebuild from different memory zones the original CHECKSUM_COMPLETE > > value. > > > > Normally the driver keeps some headroom @skb->head, so the actual mac > header starts @ skb->head + driver_specific_headroom Good to know, but this headroom isn't covered by skb->csum, so not useful here, right? The skb->csum for mlx5 only covers network header and its payload.
On Thu, 2018-11-22 at 17:45 -0800, Cong Wang wrote: > On Wed, Nov 21, 2018 at 11:33 AM Saeed Mahameed <saeedm@mellanox.com> > wrote: > > On Wed, 2018-11-21 at 10:26 -0800, Eric Dumazet wrote: > > > On Wed, Nov 21, 2018 at 10:17 AM Cong Wang < > > > xiyou.wangcong@gmail.com> > > > wrote: > > > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet < > > > > eric.dumazet@gmail.com> wrote: > > > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > > > > Currently, we only dump a few selected skb fields in > > > > > > netdev_rx_csum_fault(). It is not suffient for debugging > > > > > > checksum > > > > > > fault. This patch introduces skb_dump() which dumps skb mac > > > > > > header, > > > > > > network header and its whole skb->data too. > > > > > > > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > > > Cc: Eric Dumazet <edumazet@google.com> > > > > > > Cc: David Miller <davem@davemloft.net> > > > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > > > --- > > > > > > + print_hex_dump(level, "skb data: ", > > > > > > DUMP_PREFIX_OFFSET, > > > > > > 16, 1, > > > > > > + skb->data, skb->len, false); > > > > > > > > > > As I mentioned to David, we want all the bytes that were > > > > > maybe > > > > > already pulled > > > > > > > > > > (skb->head starting point, not skb->data) > > > > > > > > Hmm, with mac header and network header, it is effectively from > > > > skb->head, no? > > > > Is there anything between skb->head and mac header? > > > > > > Oh, I guess we wanted a single hex dump, or we need some user > > > program > > > to be able to > > > rebuild from different memory zones the original > > > CHECKSUM_COMPLETE > > > value. > > > > > > > Normally the driver keeps some headroom @skb->head, so the actual > > mac > > header starts @ skb->head + driver_specific_headroom > > Good to know, but this headroom isn't covered by skb->csum, so > not useful here, right? The skb->csum for mlx5 only covers network > header and its payload. correct
FWIW, I find the patch really useful - I applied it to my local dev repo (with minor changes) and use skb_dump() a lot now. It would be great if it makes its way into net-next in some form. On Fri, Nov 30, 2018 at 12:15 PM Saeed Mahameed <saeedm@mellanox.com> wrote: > > On Thu, 2018-11-22 at 17:45 -0800, Cong Wang wrote: > > On Wed, Nov 21, 2018 at 11:33 AM Saeed Mahameed <saeedm@mellanox.com> > > wrote: > > > On Wed, 2018-11-21 at 10:26 -0800, Eric Dumazet wrote: > > > > On Wed, Nov 21, 2018 at 10:17 AM Cong Wang < > > > > xiyou.wangcong@gmail.com> > > > > wrote: > > > > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet < > > > > > eric.dumazet@gmail.com> wrote: > > > > > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > > > > > Currently, we only dump a few selected skb fields in > > > > > > > netdev_rx_csum_fault(). It is not suffient for debugging > > > > > > > checksum > > > > > > > fault. This patch introduces skb_dump() which dumps skb mac > > > > > > > header, > > > > > > > network header and its whole skb->data too. > > > > > > > > > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > > > > Cc: Eric Dumazet <edumazet@google.com> > > > > > > > Cc: David Miller <davem@davemloft.net> > > > > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > > > > --- > > > > > > > + print_hex_dump(level, "skb data: ", > > > > > > > DUMP_PREFIX_OFFSET, > > > > > > > 16, 1, > > > > > > > + skb->data, skb->len, false); > > > > > > > > > > > > As I mentioned to David, we want all the bytes that were > > > > > > maybe > > > > > > already pulled > > > > > > > > > > > > (skb->head starting point, not skb->data) > > > > > > > > > > Hmm, with mac header and network header, it is effectively from > > > > > skb->head, no? > > > > > Is there anything between skb->head and mac header? > > > > > > > > Oh, I guess we wanted a single hex dump, or we need some user > > > > program > > > > to be able to > > > > rebuild from different memory zones the original > > > > CHECKSUM_COMPLETE > > > > value. > > > > > > > > > > Normally the driver keeps some headroom @skb->head, so the actual > > > mac > > > header starts @ skb->head + driver_specific_headroom > > > > Good to know, but this headroom isn't covered by skb->csum, so > > not useful here, right? The skb->csum for mlx5 only covers network > > header and its payload. > > correct >
On Wed, Dec 5, 2018 at 12:33 PM Peter Oskolkov <posk.devel@gmail.com> wrote: > > FWIW, I find the patch really useful - I applied it to my local dev > repo (with minor changes) and use skb_dump() a lot now. It would be > great if it makes its way into net-next in some form. +1 very informative. skb_warn_bad_offload would also benefit from more packet detail. And please consider logging device info, too. I'm currently debugging a (non-reproducible so far, so relying purely on the log message) stack trace for a packet that triggers that warning in validate_xmit_skb on the path to an unknown virtual device. > On Fri, Nov 30, 2018 at 12:15 PM Saeed Mahameed <saeedm@mellanox.com> wrote: > > > > On Thu, 2018-11-22 at 17:45 -0800, Cong Wang wrote: > > > On Wed, Nov 21, 2018 at 11:33 AM Saeed Mahameed <saeedm@mellanox.com> > > > wrote: > > > > On Wed, 2018-11-21 at 10:26 -0800, Eric Dumazet wrote: > > > > > On Wed, Nov 21, 2018 at 10:17 AM Cong Wang < > > > > > xiyou.wangcong@gmail.com> > > > > > wrote: > > > > > > On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet < > > > > > > eric.dumazet@gmail.com> wrote: > > > > > > > > > > > > > > On 11/20/2018 06:13 PM, Cong Wang wrote: > > > > > > > > Currently, we only dump a few selected skb fields in > > > > > > > > netdev_rx_csum_fault(). It is not suffient for debugging > > > > > > > > checksum > > > > > > > > fault. This patch introduces skb_dump() which dumps skb mac > > > > > > > > header, > > > > > > > > network header and its whole skb->data too. > > > > > > > > > > > > > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > > > > > Cc: Eric Dumazet <edumazet@google.com> > > > > > > > > Cc: David Miller <davem@davemloft.net> > > > > > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > > > > > --- > > > > > > > > + print_hex_dump(level, "skb data: ", > > > > > > > > DUMP_PREFIX_OFFSET, > > > > > > > > 16, 1, > > > > > > > > + skb->data, skb->len, false); > > > > > > > > > > > > > > As I mentioned to David, we want all the bytes that were > > > > > > > maybe > > > > > > > already pulled > > > > > > > > > > > > > > (skb->head starting point, not skb->data) > > > > > > > > > > > > Hmm, with mac header and network header, it is effectively from > > > > > > skb->head, no? > > > > > > Is there anything between skb->head and mac header? > > > > > > > > > > Oh, I guess we wanted a single hex dump, or we need some user > > > > > program > > > > > to be able to > > > > > rebuild from different memory zones the original > > > > > CHECKSUM_COMPLETE > > > > > value. > > > > > > > > > > > > > Normally the driver keeps some headroom @skb->head, so the actual > > > > mac > > > > header starts @ skb->head + driver_specific_headroom > > > > > > Good to know, but this headroom isn't covered by skb->csum, so > > > not useful here, right? The skb->csum for mlx5 only covers network > > > header and its payload. > > > > correct > >
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index afddb5c17ce5..844c0a7ff52f 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -4218,5 +4218,10 @@ static inline __wsum lco_csum(struct sk_buff *skb) return csum_partial(l4_hdr, csum_start - l4_hdr, partial); } +#ifdef CONFIG_BUG +void skb_dump(const char *level, const struct sk_buff *skb, bool dump_header, + bool dump_mac_header, bool dump_network_header); +#endif + #endif /* __KERNEL__ */ #endif /* _LINUX_SKBUFF_H */ diff --git a/net/core/dev.c b/net/core/dev.c index f2bfd2eda7b2..dc54c89fb4b1 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3097,11 +3097,7 @@ void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb) pr_err("%s: hw csum failure\n", dev ? dev->name : "<unknown>"); if (dev) pr_err("dev features: %pNF\n", &dev->features); - pr_err("skb len=%u data_len=%u pkt_type=%u gso_size=%u gso_type=%u nr_frags=%u ip_summed=%u csum=%x csum_complete_sw=%d csum_valid=%d csum_level=%u\n", - skb->len, skb->data_len, skb->pkt_type, - skb_shinfo(skb)->gso_size, skb_shinfo(skb)->gso_type, - skb_shinfo(skb)->nr_frags, skb->ip_summed, skb->csum, - skb->csum_complete_sw, skb->csum_valid, skb->csum_level); + skb_dump(KERN_ERR, skb, true, true, true); dump_stack(); } } diff --git a/net/core/skbuff.c b/net/core/skbuff.c index b6ba923e7dc7..21aaef3f6a4a 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -5555,3 +5555,52 @@ void skb_condense(struct sk_buff *skb) */ skb->truesize = SKB_TRUESIZE(skb_end_offset(skb)); } + +#ifdef CONFIG_BUG +void skb_dump(const char *level, const struct sk_buff *skb, bool dump_header, + bool dump_mac_header, bool dump_network_header) +{ + struct sk_buff *frag_iter; + int i; + + if (dump_header) + printk("%sskb len=%u data_len=%u pkt_type=%u gso_size=%u gso_type=%u nr_frags=%u ip_summed=%u csum=%x csum_complete_sw=%d csum_valid=%d csum_level=%u\n", + level, skb->len, skb->data_len, skb->pkt_type, + skb_shinfo(skb)->gso_size, skb_shinfo(skb)->gso_type, + skb_shinfo(skb)->nr_frags, skb->ip_summed, skb->csum, + skb->csum_complete_sw, skb->csum_valid, skb->csum_level); + + if (dump_mac_header && skb_mac_header_was_set(skb)) + print_hex_dump(level, "mac header: ", DUMP_PREFIX_OFFSET, 16, 1, + skb_mac_header(skb), skb_mac_header_len(skb), + false); + + if (dump_network_header && skb_network_header_was_set(skb)) + print_hex_dump(level, "network header: ", DUMP_PREFIX_OFFSET, + 16, 1, skb_network_header(skb), + skb_network_header_len(skb), false); + + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1, + skb->data, skb->len, false); + + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { + skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; + u32 p_off, p_len, copied; + struct page *p; + u8 *vaddr; + + skb_frag_foreach_page(frag, frag->page_offset, skb_frag_size(frag), + p, p_off, p_len, copied) { + vaddr = kmap_atomic(p); + print_hex_dump(level, "skb frag: ", DUMP_PREFIX_OFFSET, + 16, 1, vaddr + p_off, p_len, false); + kunmap_atomic(vaddr); + } + } + + if (skb_has_frag_list(skb)) + printk("%sskb frags list:\n", level); + skb_walk_frags(skb, frag_iter) + skb_dump(level, frag_iter, false, false, false); +} +#endif
Currently, we only dump a few selected skb fields in netdev_rx_csum_fault(). It is not suffient for debugging checksum fault. This patch introduces skb_dump() which dumps skb mac header, network header and its whole skb->data too. Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Eric Dumazet <edumazet@google.com> Cc: David Miller <davem@davemloft.net> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- include/linux/skbuff.h | 5 +++++ net/core/dev.c | 6 +----- net/core/skbuff.c | 49 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-)