linux-kernel - Re: [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang

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: <3bc5824f-41c3-41c4-bcfc-9cb7d6c37937@intel.com>
Date: Mon, 22 Jul 2024 11:23:54 +0200
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <apw@...onical.com>,
	<joe@...ches.com>, <dwaipayanray1@...il.com>, <lukas.bulwahn@...il.com>,
	<akpm@...ux-foundation.org>, <willemb@...gle.com>, <edumazet@...gle.com>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>, Wojciech Drewek <wojciech.drewek@...el.com>,
	Simon Horman <horms@...nel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health:
 dump also skb on Tx hang



On 7/14/2024 4:30 PM, Jakub Kicinski wrote:
> On Fri, 12 Jul 2024 05:32:51 -0400 Mateusz Polchlopek wrote:
>> +	buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> +		"skb 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",
>> +		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);
> 
> Make it a generic helper in devlink?

Makes sense for me but need to consult with an author

> 
>> +	if (dev)
>> +		buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> +					  "dev name=%s feat=%pNF\n", dev->name,
>> +					  &dev->features);
>> +	if (sk)
>> +		buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> +					  "sk family=%hu type=%u proto=%u\n",
>> +					  sk->sk_family, sk->sk_type,
>> +					  sk->sk_protocol);
>> +
>> +	if (headroom)
>> +		buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> +					      "skb headroom: ", skb->head,
>> +					      headroom);
>> +
>> +	seg_len = min_t(int, skb_headlen(skb), len);
>> +	if (seg_len)
>> +		buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> +					      "skb linear:   ", skb->data,
>> +					      seg_len);
>> +	len -= seg_len;
>> +
>> +	if (tailroom)
>> +		buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> +					      "skb tailroom: ",
>> +					      skb_tail_pointer(skb), tailroom);
> 
> The printing on tailroom, headroom and frag data seems a bit much.
> I guess you're only printing the head SKB so it may be fine. But
> I don't think it's useful. The device will probably only care about
> the contents of the headers, for other parts only the metadata matters.
> No strong preference tho.
> 
>> +	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, skb_frag_off(frag),
>> +				      skb_frag_size(frag), p, p_off, p_len,
>> +				      copied) {
>> +			seg_len = min_t(int, p_len, len);
>> +			vaddr = kmap_local_page(p);
>> +			buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> +						      "skb frag:     ",
>> +						      vaddr + p_off, seg_len);
>> +			kunmap_local(vaddr);
>> +			len -= seg_len;
>> +
>> +			if (!len || buf_pos == buf_size)
>> +				break;
>> +		}
>> +	}
>> +
>> +	if (skb_has_frag_list(skb)) {
>> +		buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> +					  "skb fraglist:\n");
>> +		skb_walk_frags(skb, list_skb) {
>> +			buf_pos = ice_skb_dump_buf(buf, buf_size, buf_pos,
>> +						   list_skb);
>> +
>> +			if (buf_pos == buf_size)
>> +				break;
>> +		}
>> +	}
> 
> You support transmitting skbs with fraglist? 🤨️