Re: [PATCH net] ionic: linearize tso skb with too many frags
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-03-16 21:55:28
On Tue, 16 Mar 2021 11:52:43 -0700 Shannon Nelson wrote:
quoted hunk ↗ jump to hunk
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. 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)