[Patch net-next v2] net: dump more useful information in netdev_rx_csum_fault()

[Patch net-next v2] net: dump more useful information in netdev_rx_csum_fault()

[Cong Wang]

Currently netdev_rx_csum_fault() only shows a device name,
we need more information about the skb for debugging csum
failures.

Sample output:

 ens3: hw csum failure
 dev features: 0x0000000000014b89
 skb len=84 data_len=0 pkt_type=0 gso_size=0 gso_type=0 nr_frags=0 ip_summed=0 csum=0 csum_complete_sw=0 csum_valid=0 csum_level=0

Note, I use pr_err() just to be consistent with the existing one.

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/netdevice.h |  5 +++--
 net/core/datagram.c       |  2 +-
 net/core/dev.c            | 11 +++++++++--
 net/core/skbuff.c         |  4 ++--
 net/sunrpc/socklib.c      |  2 +-
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 487fa5e0e165..5a97ffbff932 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4342,9 +4342,10 @@ static inline bool can_checksum_protocol(netdev_features_t features,
 }
 
 #ifdef CONFIG_BUG
-void netdev_rx_csum_fault(struct net_device *dev);
+void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb);
 #else
-static inline void netdev_rx_csum_fault(struct net_device *dev)
+static inline void netdev_rx_csum_fault(struct net_device *dev,
+					struct sk_buff *skb)
 {
 }
 #endif
diff --git a/net/core/datagram.c b/net/core/datagram.c
index 07983b90d2bd..4bf62b1afa3b 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -767,7 +767,7 @@ int skb_copy_and_csum_datagram_msg(struct sk_buff *skb,
 
 		if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
 		    !skb->csum_complete_sw)
-			netdev_rx_csum_fault(NULL);
+			netdev_rx_csum_fault(NULL, skb);
 	}
 	return 0;
 fault:
diff --git a/net/core/dev.c b/net/core/dev.c
index bf7e0a471186..5927f6a7c301 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3091,10 +3091,17 @@ EXPORT_SYMBOL(__skb_gso_segment);
 
 /* Take action when hardware reception checksum errors are detected. */
 #ifdef CONFIG_BUG
-void netdev_rx_csum_fault(struct net_device *dev)
+void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb)
 {
 	if (net_ratelimit()) {
 		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);
 		dump_stack();
 	}
 }
@@ -5781,7 +5788,7 @@ __sum16 __skb_gro_checksum_complete(struct sk_buff *skb)
 	if (likely(!sum)) {
 		if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
 		    !skb->csum_complete_sw)
-			netdev_rx_csum_fault(skb->dev);
+			netdev_rx_csum_fault(skb->dev, skb);
 	}
 
 	NAPI_GRO_CB(skb)->csum = wsum;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 396fcb3baad0..fcb1155a00ec 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2653,7 +2653,7 @@ __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len)
 	if (likely(!sum)) {
 		if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
 		    !skb->csum_complete_sw)
-			netdev_rx_csum_fault(skb->dev);
+			netdev_rx_csum_fault(skb->dev, skb);
 	}
 	if (!skb_shared(skb))
 		skb->csum_valid = !sum;
@@ -2673,7 +2673,7 @@ __sum16 __skb_checksum_complete(struct sk_buff *skb)
 	if (likely(!sum)) {
 		if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
 		    !skb->csum_complete_sw)
-			netdev_rx_csum_fault(skb->dev);
+			netdev_rx_csum_fault(skb->dev, skb);
 	}
 
 	if (!skb_shared(skb)) {
diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
index 9062967575c4..7e55cfc69697 100644
--- a/net/sunrpc/socklib.c
+++ b/net/sunrpc/socklib.c
@@ -175,7 +175,7 @@ int csum_partial_copy_to_xdr(struct xdr_buf *xdr, struct sk_buff *skb)
 		return -1;
 	if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
 	    !skb->csum_complete_sw)
-		netdev_rx_csum_fault(skb->dev);
+		netdev_rx_csum_fault(skb->dev, skb);
 	return 0;
 no_checksum:
 	if (xdr_partial_copy_from_skb(xdr, 0, &desc, xdr_skb_read_bits) < 0)
-- 
2.19.1

Re: [Patch net-next v2] net: dump more useful information in netdev_rx_csum_fault()

[Willem de Bruijn]

On Mon, Nov 12, 2018 at 2:49 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Currently netdev_rx_csum_fault() only shows a device name,
> we need more information about the skb for debugging csum
> failures.
>
> Sample output:
>
>  ens3: hw csum failure
>  dev features: 0x0000000000014b89
>  skb len=84 data_len=0 pkt_type=0 gso_size=0 gso_type=0 nr_frags=0 ip_summed=0 csum=0 csum_complete_sw=0 csum_valid=0 csum_level=0

Recent issues were protocol dependent, including whether vlan headers
were present. Perhaps also print skb vlan fields and even the first N
bytes of data to inspect protocol headers? Also skb_iif, esp. if this
differs from dev->ifindex.

Re: [Patch net-next v2] net: dump more useful information in netdev_rx_csum_fault()

[Cong Wang]

On Tue, Nov 13, 2018 at 10:01 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, Nov 12, 2018 at 2:49 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > Currently netdev_rx_csum_fault() only shows a device name,
> > we need more information about the skb for debugging csum
> > failures.
> >
> > Sample output:
> >
> >  ens3: hw csum failure
> >  dev features: 0x0000000000014b89
> >  skb len=84 data_len=0 pkt_type=0 gso_size=0 gso_type=0 nr_frags=0 ip_summed=0 csum=0 csum_complete_sw=0 csum_valid=0 csum_level=0
>
> Recent issues were protocol dependent, including whether vlan headers
> were present. Perhaps also print skb vlan fields and even the first N
> bytes of data to inspect protocol headers? Also skb_iif, esp. if this
> differs from dev->ifindex.

Pawel's case seems to be vlan related, however, as I mentioned,
my case is neither vlan nor RXFCS related.

Ideally, we should dump the whole packet in order to verify the
correctness of the checksum. :) It is not easy to do so given
how complex an skb is now. This is why I only select a few skb
fields to dump. I am pretty sure this can't cover all cases, you
can always add more for your need in the future.

Thanks.

Re: [Patch net-next v2] net: dump more useful information in netdev_rx_csum_fault()

[Willem de Bruijn]

On Tue, Nov 13, 2018 at 10:14 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, Nov 13, 2018 at 10:01 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Mon, Nov 12, 2018 at 2:49 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > Currently netdev_rx_csum_fault() only shows a device name,
> > > we need more information about the skb for debugging csum
> > > failures.
> > >
> > > Sample output:
> > >
> > >  ens3: hw csum failure
> > >  dev features: 0x0000000000014b89
> > >  skb len=84 data_len=0 pkt_type=0 gso_size=0 gso_type=0 nr_frags=0 ip_summed=0 csum=0 csum_complete_sw=0 csum_valid=0 csum_level=0
> >
> > Recent issues were protocol dependent, including whether vlan headers
> > were present. Perhaps also print skb vlan fields and even the first N
> > bytes of data to inspect protocol headers? Also skb_iif, esp. if this
> > differs from dev->ifindex.
>
> Pawel's case seems to be vlan related, however, as I mentioned,
> my case is neither vlan nor RXFCS related.
>
> Ideally, we should dump the whole packet in order to verify the
> correctness of the checksum. :) It is not easy to do so given
> how complex an skb is now. This is why I only select a few skb
> fields to dump. I am pretty sure this can't cover all cases, you
> can always add more for your need in the future.

Sounds good. This patch is definitely useful as is.

Also, instead of adding code to print a (partial) packet header to
the kernel log, it may make more sense to export the skb to a
drop monitor over netlink or a perf buffer.

Re: [Patch net-next v2] net: dump more useful information in netdev_rx_csum_fault()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 12 Nov 2018 14:47:18 -0800

> Currently netdev_rx_csum_fault() only shows a device name,
> we need more information about the skb for debugging csum
> failures.
> 
> Sample output:
> 
>  ens3: hw csum failure
>  dev features: 0x0000000000014b89
>  skb len=84 data_len=0 pkt_type=0 gso_size=0 gso_type=0 nr_frags=0 ip_summed=0 csum=0 csum_complete_sw=0 csum_valid=0 csum_level=0
> 
> Note, I use pr_err() just to be consistent with the existing one.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks Cong.