netdev - Re: [Patch net-next] net: dump more useful information in netdev_rx_csum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM_iQpWryiOQg23fZ2iveM25Rc-V8i5uvph4ezsD2dimqF4doA@mail.gmail.com>
Date:   Fri, 9 Nov 2018 18:09:55 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     linyunsheng@...wei.com
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()

On Fri, Nov 9, 2018 at 6:02 PM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2018/11/10 9:42, Cong Wang wrote:
> > On Fri, Nov 9, 2018 at 5:39 PM Yunsheng Lin <linyunsheng@...wei.com> wrote:
> >>
> >> On 2018/11/10 3:43, Cong Wang wrote:
> >>> Currently netdev_rx_csum_fault() only shows a device name,
> >>> we need more information about the skb for debugging.
> >>>
> >>> Sample output:
> >>>
> >>>  ens3: hw csum failure
> >>>  dev features: 0x0000000000014b89
> >>>  skb len=84 data_len=0 gso_size=0 gso_type=0 ip_summed=0 csum=0, csum_complete_sw=0, csum_valid=0
> >>>
> >>> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> >>> ---
> >>>  include/linux/netdevice.h |  5 +++--
> >>>  net/core/datagram.c       |  6 +++---
> >>>  net/core/dev.c            | 10 ++++++++--
> >>>  net/sunrpc/socklib.c      |  2 +-
> >>>  4 files changed, 15 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>> index 857f8abf7b91..fabcd9fa6cf7 100644
> >>> --- a/include/linux/netdevice.h
> >>> +++ b/include/linux/netdevice.h
> >>> @@ -4332,9 +4332,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 57f3a6fcfc1e..d8f4d55cd6c5 100644
> >>> --- a/net/core/datagram.c
> >>> +++ b/net/core/datagram.c
> >>> @@ -736,7 +736,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;
> >>> @@ -756,7 +756,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)) {
> >>> @@ -810,7 +810,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 0ffcbdd55fa9..2b337df26117 100644
> >>> --- a/net/core/dev.c
> >>> +++ b/net/core/dev.c
> >>> @@ -3091,10 +3091,16 @@ 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=%d data_len=%d gso_size=%d gso_type=%d ip_summed=%d csum=%x, csum_complete_sw=%d, csum_valid=%d\n",
> >>> +                    skb->len, skb->data_len, skb_shinfo(skb)->gso_size,
> >>> +                    skb_shinfo(skb)->gso_type, skb->ip_summed, skb->csum,
> >>> +                    skb->csum_complete_sw, skb->csum_valid);
> >>
> >>
> >> This function also have the netdev available, use netdev_err to log the error?
> >
> > It is apparently not me who picked pr_err() from the beginning,
> > I just follow that pr_err(). If you are not happy with it, please send
> > a followup.
>
> Yes, but perhaps it is something to improve.


Sure, no one stops you from improving it in a followup patch. :)


> When using the netdev, then maybe it does not have to check if dev is null, because
> netdev_err has handled the netdev being NULL case.
> Maybe I missed something that netdev can not be used here?
> If not, maybe I can send a followup.
>

Maybe. Again, my patch intends to add a few debugging logs,
not to convert pr_err() to whatever else, they are totally different
goals. I choose pr_err() only because I follow the existing one,
not to say which one is better than the other.

Thanks.