| Message ID | 20210316185243.30053-1-snelson@pensando.io (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ionic: linearize tso skb with too many frags | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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)
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)
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)
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(-)