| 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
| ||
|
Date: Wed, 3 Jul 2019 10:55:18 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: Network Development <netdev@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Cong Wang <xiyou.wangcong@...il.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
Eric Dumazet <eric.dumazet@...il.com>,
Saeed Mahameed <saeedm@...lanox.com>,
Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next] skbuff: increase verbosity when dumping skb data
On Tue, Jul 2, 2019 at 11:28 PM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2019/7/3 3:39, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > skb_warn_bad_offload and netdev_rx_csum_fault trigger on hard to debug
> > issues. Dump more state and the header.
> >
> > Optionally dump the entire packet and linear segment. This is required
> > to debug checksum bugs that may include bytes past skb_tail_pointer().
> >
> > Both call sites call this function inside a net_ratelimit() block.
> > Limit full packet log further to a hard limit of can_dump_full (5).
> >
> > Based on an earlier patch by Cong Wang, see link below.
> >
> > Link: https://patchwork.ozlabs.org/patch/1000841/
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > ---
> > +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;
> > + struct sk_buff *list_skb;
> > + bool has_mac, has_trans;
> > + 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;
>
> Minor question:
> Here we set the len to skb->len if full_pkt is true when skb_dump is
> called with frag_list skb and full_pkt being true below, which may
> cause some problem?
Good catch, thanks!
That recursive call to skb_dump on the frag_list below was not updated
from a previous revision that passed an explicit length.
> Maybe change the definition to:
> void skb_dump(const char *level, const struct sk_buff *skb, int len, bool full_pkt)
Indeed. It is less important when full_pkt, as then the entire
frag_list will be printed.
But if len is truncated, but somehow len != 0 when reaching the
frag_list, it might overshoot the limit. Will fix for v2.
> skb_dump(KERN_ERR, skb, skb->len, true);
>
> > + else
> > + len = min_t(int, skb->len, MAX_HEADER + 128);
> > +
> > + headroom = skb_headroom(skb);
> > + tailroom = skb_tailroom(skb);
> > +
> > + has_mac = skb_mac_header_was_set(skb);
> > + has_trans = skb_transport_header_was_set(skb);
> > +
> > + printk("%sskb len=%u headroom=%u headlen=%u tailroom=%u\n"
> > + "mac=(%d,%d) net=(%d,%d) trans=%d\n"
> > + "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
> > + "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
> > + "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
> > + level, skb->len, headroom, skb_headlen(skb), tailroom,
> > + has_mac ? skb->mac_header : -1,
> > + has_mac ? skb_mac_header_len(skb) : -1,
> > + skb->network_header,
> > + has_trans ? skb_network_header_len(skb) : -1,
> > + has_trans ? skb->transport_header : -1,
> > + sh->tx_flags, sh->nr_frags,
> > + sh->gso_size, sh->gso_type, sh->gso_segs,
> > + skb->csum, skb->ip_summed, skb->csum_complete_sw,
> > + skb->csum_valid, skb->csum_level,
> > + skb->hash, skb->sw_hash, skb->l4_hash,
> > + ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
> > +
> > + if (dev)
> > + printk("%sdev name=%s feat=0x%pNF\n",
> > + level, dev->name, &dev->features);
> > + if (sk)
> > + printk("%ssk family=%hu type=%hu proto=%hu\n",
> > + level, sk->sk_family, sk->sk_type, sk->sk_protocol);
> > +
> > + if (full_pkt && headroom)
> > + print_hex_dump(level, "skb headroom: ", DUMP_PREFIX_OFFSET,
> > + 16, 1, skb->head, headroom, false);
> > +
> > + seg_len = min_t(int, skb_headlen(skb), len);
> > + if (seg_len)
> > + print_hex_dump(level, "skb linear: ", DUMP_PREFIX_OFFSET,
> > + 16, 1, skb->data, seg_len, false);
> > + len -= seg_len;
> > +
> > + if (full_pkt && tailroom)
> > + print_hex_dump(level, "skb tailroom: ", DUMP_PREFIX_OFFSET,
> > + 16, 1, skb_tail_pointer(skb), tailroom, false);
> > +
> > + for (i = 0; len && 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) {
> > + seg_len = min_t(int, p_len, len);
> > + vaddr = kmap_atomic(p);
> > + print_hex_dump(level, "skb frag: ",
> > + DUMP_PREFIX_OFFSET,
> > + 16, 1, vaddr + p_off, seg_len, false);
> > + kunmap_atomic(vaddr);
> > + len -= seg_len;
> > + if (!len)
> > + break;
> > + }
> > + }
> > +
> > + if (len && skb_has_frag_list(skb)) {
> > + printk("skb fraglist:\n");
> > + skb_walk_frags(skb, list_skb) {
> > + if (len <= 0)
> > + break;
> > + skb_dump(level, list_skb, len);
>
> Here we call skb_dump passing len as full_pkt.
>
> Maybe call it with skb_dump(level, list_skb, len, full_pkt);
>
> > + len -= list_skb->len;
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL(skb_dump);
Powered by blists - more mailing lists