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

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

[Shannon Nelson]

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@pensando.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;
+	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;
 }
 
 static int ionic_maybe_stop_tx(struct ionic_queue *q, int ndescs)
-- 
2.17.1

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

[Jakub Kicinski]

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@pensando.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.

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

> +	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.

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

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

[Shannon Nelson]

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@pensando.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)