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

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 <redacted>
---
 .../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

On Tue, 16 Mar 2021 11:52:43 -0700 Shannon Nelson wrote:

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)

On 3/16/21 2:54 PM, Jakub Kicinski wrote:

On Tue, 16 Mar 2021 11:52:43 -0700 Shannon Nelson wrote:

quoted

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 <redacted>
---
  .../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.

quoted

+	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

quoted

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