netdev - Re: [PATCH net] ionic: linearize tso skb with too many frags

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <567bc95f-568a-4413-b559-a1ec309f34af@pensando.io>
Date:   Tue, 16 Mar 2021 16:24:33 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, drivers@...sando.io
Subject: Re: [PATCH net] ionic: linearize tso skb with too many frags

On 3/16/21 2:54 PM, Jakub Kicinski wrote:
> On Tue, 16 Mar 2021 11:52:43 -0700 Shannon Nelson wrote:
>> We were linearizing non-TSO skbs that had too many frags, but
>> we weren't checking number of frags on TSO skbs.  This could
>> lead to a bad page reference when we received a TSO skb with
>> more frags than the Tx descriptor could support.
>>
>> Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling")
>> Signed-off-by: Shannon Nelson <snelson@...sando.io>
>> ---
>>   .../net/ethernet/pensando/ionic/ionic_txrx.c  | 28 ++++++++++---------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> index 162a1ff1e9d2..462b0d106be4 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> @@ -1079,25 +1079,27 @@ static int ionic_tx_descs_needed(struct ionic_queue *q, struct sk_buff *skb)
>>   {
>>   	int sg_elems = q->lif->qtype_info[IONIC_QTYPE_TXQ].max_sg_elems;
>>   	struct ionic_tx_stats *stats = q_to_tx_stats(q);
>> +	int ndescs;
>>   	int err;
>>   
>> -	/* If TSO, need roundup(skb->len/mss) descs */
>> +	/* If TSO, need roundup(skb->len/mss) descs
>> +	 * If non-TSO, just need 1 desc and nr_frags sg elems
>> +	 */
>>   	if (skb_is_gso(skb))
>> -		return (skb->len / skb_shinfo(skb)->gso_size) + 1;
>> +		ndescs = (skb->len / skb_shinfo(skb)->gso_size) + 1;
> Slightly unrelated but why not gso_segs? len / gso_size + 1 could be
> over counting, not to mention that div is expensive.

Good catch - we can probably do that.

>
> Are you segmenting in the driver? Why do you need #segs descriptors?

The device needs each descriptor to be no more than mss length, so there 
might be a number of descriptors for a large packet.

>
>> +	else
>> +		ndescs = 1;
>>   
>> -	/* If non-TSO, just need 1 desc and nr_frags sg elems */
>> -	if (skb_shinfo(skb)->nr_frags <= sg_elems)
>> -		return 1;
>> +	/* If too many frags, linearize */
>> +	if (skb_shinfo(skb)->nr_frags > sg_elems) {
>> +		err = skb_linearize(skb);
>> +		if (err)
>> +			return err;
>>   
>> -	/* Too many frags, so linearize */
>> -	err = skb_linearize(skb);
>> -	if (err)
>> -		return err;
>> -
>> -	stats->linearize++;
>> +		stats->linearize++;
>> +	}
>>   
>> -	/* Need 1 desc and zero sg elems */
>> -	return 1;
>> +	return ndescs;
> I'd be tempted to push back on the refactoring here, you could've
> just replaced return 1;s with return ndescs;s without changing
> the indentation.. this will give all backporters a pause. But
> not the end of the world, I guess.

I can tweak that a little.

I'll send a v2.

sln

>
>>   }
>>   
>>   static int ionic_maybe_stop_tx(struct ionic_queue *q, int ndescs)