[Patch net-next 1/2] net: introduce skb_network_header_was_set()

[Patch net-next 1/2] net: introduce skb_network_header_was_set()

[Cong Wang]

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/skbuff.h | 5 +++++
 net/core/skbuff.c      | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a2e8297a5b00..afddb5c17ce5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2444,6 +2444,11 @@ static inline u32 skb_network_header_len(const struct sk_buff *skb)
 	return skb->transport_header - skb->network_header;
 }
 
+static inline int skb_network_header_was_set(const struct sk_buff *skb)
+{
+	return skb->network_header != (typeof(skb->network_header))~0U;
+}
+
 static inline u32 skb_inner_network_header_len(const struct sk_buff *skb)
 {
 	return skb->inner_transport_header - skb->inner_network_header;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9a8a72cefe9b..b6ba923e7dc7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -227,6 +227,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	skb_reset_tail_pointer(skb);
 	skb->end = skb->tail + size;
 	skb->mac_header = (typeof(skb->mac_header))~0U;
+	skb->network_header = (typeof(skb->network_header))~0U;
 	skb->transport_header = (typeof(skb->transport_header))~0U;
 
 	/* make sure we initialize shinfo sequentially */
@@ -292,6 +293,7 @@ struct sk_buff *__build_skb(void *data, unsigned int frag_size)
 	skb_reset_tail_pointer(skb);
 	skb->end = skb->tail + size;
 	skb->mac_header = (typeof(skb->mac_header))~0U;
+	skb->network_header = (typeof(skb->network_header))~0U;
 	skb->transport_header = (typeof(skb->transport_header))~0U;
 
 	/* make sure we initialize shinfo sequentially */
-- 
2.19.1

[Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Cong Wang]

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(-)

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
-- 
2.19.1

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Eric Dumazet]

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.

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Cong Wang]

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

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Eric Dumazet]

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.

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Saeed Mahameed]

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.

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Cong Wang]

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.

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Saeed Mahameed]

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

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Peter Oskolkov]

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
>

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Willem de Bruijn]

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
> >

Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()

[Cong Wang]

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.