[PATCH net-next] net: always dump full packets with skb_dump

[PATCH net-next] net: always dump full packets with skb_dump

[Vladimir Oltean]

Currently skb_dump has a restriction to only dump full packet for the
first 5 socket buffers, then only headers will be printed. Remove this
arbitrary and confusing restriction, which is only documented vaguely
("up to") in the comments above the prototype.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/core/skbuff.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e0774471f56d..720076a6e2b1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -712,11 +712,10 @@ EXPORT_SYMBOL(kfree_skb_list);
  *
  * Must only be called from net_ratelimit()-ed paths.
  *
- * Dumps up to can_dump_full whole packets if full_pkt, headers otherwise.
+ * Dumps whole packets if full_pkt, only headers otherwise.
  */
 void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
 {
-	static atomic_t can_dump_full = ATOMIC_INIT(5);
 	struct skb_shared_info *sh = skb_shinfo(skb);
 	struct net_device *dev = skb->dev;
 	struct sock *sk = skb->sk;
@@ -725,9 +724,6 @@ void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
 	int headroom, tailroom;
 	int i, len, seg_len;
 
-	if (full_pkt)
-		full_pkt = atomic_dec_if_positive(&can_dump_full) >= 0;
-
 	if (full_pkt)
 		len = skb->len;
 	else
-- 
2.25.1

Re: [PATCH net-next] net: always dump full packets with skb_dump

[Jacob Keller]

On 10/5/2020 7:48 AM, Vladimir Oltean wrote:
> Currently skb_dump has a restriction to only dump full packet for the
> first 5 socket buffers, then only headers will be printed. Remove this
> arbitrary and confusing restriction, which is only documented vaguely
> ("up to") in the comments above the prototype.
> 

So, this limitation appeared very clearly in the original commit,
6413139dfc64 ("skbuff: increase verbosity when dumping skb data")..

Searching the netdev list, that patch links back to this one as the
original idea:

https://patchwork.ozlabs.org/project/netdev/patch/20181121021309.6595-2-xiyou.wangcong@gmail.com/

I can't find any further justification on that limit. I suppose the
primary reasoning being if you somehow call this function in a loop this
would avoid dumping the entire packet over and over?

Thanks,
Jake

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/core/skbuff.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index e0774471f56d..720076a6e2b1 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -712,11 +712,10 @@ EXPORT_SYMBOL(kfree_skb_list);
>   *
>   * Must only be called from net_ratelimit()-ed paths.
>   *
> - * Dumps up to can_dump_full whole packets if full_pkt, headers otherwise.
> + * Dumps whole packets if full_pkt, only headers otherwise.
>   */
>  void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
>  {
> -	static atomic_t can_dump_full = ATOMIC_INIT(5);
>  	struct skb_shared_info *sh = skb_shinfo(skb);
>  	struct net_device *dev = skb->dev;
>  	struct sock *sk = skb->sk;
> @@ -725,9 +724,6 @@ void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
>  	int headroom, tailroom;
>  	int i, len, seg_len;
>  
> -	if (full_pkt)
> -		full_pkt = atomic_dec_if_positive(&can_dump_full) >= 0;
> -
>  	if (full_pkt)
>  		len = skb->len;
>  	else
> 

Re: [PATCH net-next] net: always dump full packets with skb_dump

[Willem de Bruijn]

On Mon, Oct 5, 2020 at 8:25 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
> On 10/5/2020 7:48 AM, Vladimir Oltean wrote:
> > Currently skb_dump has a restriction to only dump full packet for the
> > first 5 socket buffers, then only headers will be printed. Remove this
> > arbitrary and confusing restriction, which is only documented vaguely
> > ("up to") in the comments above the prototype.
> >
>
> So, this limitation appeared very clearly in the original commit,
> 6413139dfc64 ("skbuff: increase verbosity when dumping skb data")..
>
> Searching the netdev list, that patch links back to this one as the
> original idea:
>
> https://patchwork.ozlabs.org/project/netdev/patch/20181121021309.6595-2-xiyou.wangcong@gmail.com/
>
> I can't find any further justification on that limit. I suppose the
> primary reasoning being if you somehow call this function in a loop this
> would avoid dumping the entire packet over and over?

Not in a loop per se, but indeed to avoid unbounded writing to the kernel log.

skb_dump is called from skb_warn_bad_offload and netdev_rx_csum_fault.
Previously when these were triggered, a few example bad packets were
sufficient to debug the issue.

A full dump can add a lot of data to the kernel log, so I limited to
what is strictly needed.

Re: [PATCH net-next] net: always dump full packets with skb_dump

[Vladimir Oltean]

On Tue, Oct 06, 2020 at 07:30:13AM -0400, Willem de Bruijn wrote:
> skb_dump is called from skb_warn_bad_offload and netdev_rx_csum_fault.
> Previously when these were triggered, a few example bad packets were
> sufficient to debug the issue.

Yes, and it's only netdev_rx_csum_fault that matters, because
skb_warn_bad_offload calls with full_pkt=false anyway.

During the times when I had netdev_rx_csum_fault triggered, it was
pretty bad anyway. I don't think that full_pkt getting unset after 5
skbs made too big of a difference.

> A full dump can add a lot of data to the kernel log, so I limited to
> what is strictly needed.

Yes, well my expectation is that other people are using skb_dump for
debugging, even beyond those 2 callers in the mainline kernel. And when
they want to dump with full_pkt=true, they really want to dump with
full_pkt=true.

Thanks,
-Vladimir

Re: [PATCH net-next] net: always dump full packets with skb_dump

[Willem de Bruijn]

On Tue, Oct 6, 2020 at 7:43 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Tue, Oct 06, 2020 at 07:30:13AM -0400, Willem de Bruijn wrote:
> > skb_dump is called from skb_warn_bad_offload and netdev_rx_csum_fault.
> > Previously when these were triggered, a few example bad packets were
> > sufficient to debug the issue.
>
> Yes, and it's only netdev_rx_csum_fault that matters, because
> skb_warn_bad_offload calls with full_pkt=false anyway.
>
> During the times when I had netdev_rx_csum_fault triggered, it was
> pretty bad anyway. I don't think that full_pkt getting unset after 5
> skbs made too big of a difference.
>
> > A full dump can add a lot of data to the kernel log, so I limited to
> > what is strictly needed.
>
> Yes, well my expectation is that other people are using skb_dump for
> debugging, even beyond those 2 callers in the mainline kernel. And when
> they want to dump with full_pkt=true, they really want to dump with
> full_pkt=true.

Sure, that makes sense.

Re: [PATCH net-next] net: always dump full packets with skb_dump

[David Miller]

From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon,  5 Oct 2020 17:48:38 +0300

> Currently skb_dump has a restriction to only dump full packet for the
> first 5 socket buffers, then only headers will be printed. Remove this
> arbitrary and confusing restriction, which is only documented vaguely
> ("up to") in the comments above the prototype.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Applied, thank you.