netdev - Re: [PATCH iwl-next 12/12] idpf: use libeth Rx buffer management for payload buffer

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]

Date: Thu, 13 Jun 2024 13:05:58 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Simon Horman <horms@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Mina Almasry <almasrymina@...gle.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@...el.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH iwl-next 12/12] idpf: use libeth Rx buffer management for
 payload buffer

From: Simon Horman <horms@...nel.org>
Date: Sat, 1 Jun 2024 10:08:42 +0100

> + Dan Carpenter
> 
> On Tue, May 28, 2024 at 03:48:46PM +0200, Alexander Lobakin wrote:
>> idpf uses Page Pool for data buffers with hardcoded buffer lengths of
>> 4k for "classic" buffers and 2k for "short" ones. This is not flexible
>> and does not ensure optimal memory usage. Why would you need 4k buffers
>> when the MTU is 1500?
>> Use libeth for the data buffers and don't hardcode any buffer sizes. Let
>> them be calculated from the MTU for "classics" and then divide the
>> truesize by 2 for "short" ones. The memory usage is now greatly reduced
>> and 2 buffer queues starts make sense: on frames <= 1024, you'll recycle
>> (and resync) a page only after 4 HW writes rather than two.
>>
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> 
> ...
> 
> Hi Alexander,
> 
> The code above the hunk below, starting at line 3321, is:
> 
> 		if (unlikely(!hdr_len && !skb)) {
> 			hdr_len = idpf_rx_hsplit_wa(hdr, rx_buf, pkt_len);
> 			pkt_len -= hdr_len;
> 			u64_stats_update_begin(&rxq->stats_sync);
> 			u64_stats_inc(&rxq->q_stats.hsplit_buf_ovf);
> 			u64_stats_update_end(&rxq->stats_sync);
> 		}
> 		if (libeth_rx_sync_for_cpu(hdr, hdr_len)) {
> 			skb = idpf_rx_build_skb(hdr, hdr_len);
> 			if (!skb)
> 				break;
> 			u64_stats_update_begin(&rxq->stats_sync);
> 			u64_stats_inc(&rxq->q_stats.hsplit_pkts);
> 			u64_stats_update_end(&rxq->stats_sync);
> 		}
> 
>> @@ -3413,24 +3340,24 @@ static int idpf_rx_splitq_clean(struct idpf_rx_queue *rxq, int budget)
>>  		hdr->page = NULL;
>>  
>>  payload:
>> -		if (pkt_len) {
>> -			idpf_rx_sync_for_cpu(rx_buf, pkt_len);
>> -			if (skb)
>> -				idpf_rx_add_frag(rx_buf, skb, pkt_len);
>> -			else
>> -				skb = idpf_rx_construct_skb(rxq, rx_buf,
>> -							    pkt_len);
>> -		} else {
>> -			idpf_rx_put_page(rx_buf);
>> -		}
>> +		if (!libeth_rx_sync_for_cpu(rx_buf, pkt_len))
>> +			goto skip_data;
>> +
>> +		if (skb)
>> +			idpf_rx_add_frag(rx_buf, skb, pkt_len);
>> +		else
>> +			skb = idpf_rx_build_skb(rx_buf, pkt_len);
>>  
>>  		/* exit if we failed to retrieve a buffer */
>>  		if (!skb)
>>  			break;
>>  
>> -		idpf_rx_post_buf_refill(refillq, buf_id);
>> +skip_data:
>> +		rx_buf->page = NULL;
>>  
>> +		idpf_rx_post_buf_refill(refillq, buf_id);
>>  		IDPF_RX_BUMP_NTC(rxq, ntc);
>> +
>>  		/* skip if it is non EOP desc */
>>  		if (!idpf_rx_splitq_is_eop(rx_desc))
>>  			continue;
> 
> The code following this hunk, ending at line 3372, looks like this:
> 
> 		/* pad skb if needed (to make valid ethernet frame) */
> 		if (eth_skb_pad(skb)) {
> 			skb = NULL;
> 			continue;
> 		}
> 		/* probably a little skewed due to removing CRC */
> 		total_rx_bytes += skb->len;
> 
> Smatch warns that:
> .../idpf_txrx.c:3372 idpf_rx_splitq_clean() error: we previously assumed 'skb' could be null (see line 3321)
> 
> I think, but am not sure, this is because it thinks skb might
> be NULL at the point where "goto skip_data;" is now called above.
> 
> Could you look into this?

This is actually a good catch. skb indeed could be NULL and we needed to
check that in the same condition where !eop is checked.
Fixed already in my tree, so it will be fixed in v2. Thanks for catching!

(BTW I fixed that in iavf when submitting the libeth series, but forgot
 to fix that here lol >_<)
(Also, it was implicitly fixed in the later commits where I convert skb
 to xdp_buff here, so I didn't catch this one)

Thanks,
Olek