| Message ID | 20240909160604.1148178-1-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dpaa: Pad packets to ETH_ZLEN | expand |
On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@linux.dev> wrote: > > When sending packets under 60 bytes, up to three bytes of the buffer following > the data may be leaked. Avoid this by extending all packets to ETH_ZLEN, > ensuring nothing is leaked in the padding. This bug can be reproduced by > running > > $ ping -s 11 destination > > Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet") > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > --- > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index cfe6b57b1da0..e4e8ee8b7356 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) > } > #endif > > + /* Packet data is always read as 32-bit words, so zero out any part of > + * the skb which might be sent if we have to pad the packet > + */ > + if (__skb_put_padto(skb, ETH_ZLEN, false)) > + goto enomem; > + This call might linearize the packet. @nonlinear variable might be wrong after this point. > if (nonlinear) { > /* Just create a S/G fd based on the skb */ > err = skb_to_sg_fd(priv, skb, &fd); > -- > 2.35.1.1320.gc452695387.dirty > Perhaps this instead ? diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index cfe6b57b1da0e45613ac1bbf32ddd6ace329f4fd..5763d2f1bf8dd31b80fda0681361514dad1dc307 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -2272,12 +2272,12 @@ static netdev_tx_t dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) { const int queue_mapping = skb_get_queue_mapping(skb); - bool nonlinear = skb_is_nonlinear(skb); struct rtnl_link_stats64 *percpu_stats; struct dpaa_percpu_priv *percpu_priv; struct netdev_queue *txq; struct dpaa_priv *priv; struct qm_fd fd; + bool nonlinear; int offset = 0; int err = 0; @@ -2287,6 +2287,10 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) qm_fd_clear_fd(&fd); + if (__skb_put_padto(skb, ETH_ZLEN, false)) + goto enomem; + + nonlinear = skb_is_nonlinear(skb); if (!nonlinear) { /* We're going to store the skb backpointer at the beginning * of the data buffer, so we need a privately owned skb
On 9/9/24 12:46, Eric Dumazet wrote: > On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@linux.dev> wrote: >> >> When sending packets under 60 bytes, up to three bytes of the buffer following >> the data may be leaked. Avoid this by extending all packets to ETH_ZLEN, >> ensuring nothing is leaked in the padding. This bug can be reproduced by >> running >> >> $ ping -s 11 destination >> >> Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet") >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> --- >> >> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> index cfe6b57b1da0..e4e8ee8b7356 100644 >> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) >> } >> #endif >> >> + /* Packet data is always read as 32-bit words, so zero out any part of >> + * the skb which might be sent if we have to pad the packet >> + */ >> + if (__skb_put_padto(skb, ETH_ZLEN, false)) >> + goto enomem; >> + > > This call might linearize the packet. > > @nonlinear variable might be wrong after this point. > >> if (nonlinear) { >> /* Just create a S/G fd based on the skb */ >> err = skb_to_sg_fd(priv, skb, &fd); >> -- >> 2.35.1.1320.gc452695387.dirty >> > > Perhaps this instead ? > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index cfe6b57b1da0e45613ac1bbf32ddd6ace329f4fd..5763d2f1bf8dd31b80fda0681361514dad1dc307 > 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2272,12 +2272,12 @@ static netdev_tx_t > dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) > { > const int queue_mapping = skb_get_queue_mapping(skb); > - bool nonlinear = skb_is_nonlinear(skb); > struct rtnl_link_stats64 *percpu_stats; > struct dpaa_percpu_priv *percpu_priv; > struct netdev_queue *txq; > struct dpaa_priv *priv; > struct qm_fd fd; > + bool nonlinear; > int offset = 0; > int err = 0; > > @@ -2287,6 +2287,10 @@ dpaa_start_xmit(struct sk_buff *skb, struct > net_device *net_dev) > > qm_fd_clear_fd(&fd); > > + if (__skb_put_padto(skb, ETH_ZLEN, false)) > + goto enomem; > + > + nonlinear = skb_is_nonlinear(skb); > if (!nonlinear) { > /* We're going to store the skb backpointer at the beginning > * of the data buffer, so we need a privately owned skb Thanks for the suggestion; I was having a hard time figuring out where to call this. Do you have any hints for how to test this for correctness? I'm not sure how to generate a non-linear packet under 60 bytes. --Sean
On Mon, Sep 9, 2024 at 7:07 PM Sean Anderson <sean.anderson@linux.dev> wrote: > > On 9/9/24 12:46, Eric Dumazet wrote: > > On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@linux.dev> wrote: > >> > >> When sending packets under 60 bytes, up to three bytes of the buffer following > >> the data may be leaked. Avoid this by extending all packets to ETH_ZLEN, > >> ensuring nothing is leaked in the padding. This bug can be reproduced by > >> running > >> > >> $ ping -s 11 destination > >> > >> Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet") > >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > >> --- > >> > >> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > >> index cfe6b57b1da0..e4e8ee8b7356 100644 > >> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > >> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > >> @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) > >> } > >> #endif > >> > >> + /* Packet data is always read as 32-bit words, so zero out any part of > >> + * the skb which might be sent if we have to pad the packet > >> + */ > >> + if (__skb_put_padto(skb, ETH_ZLEN, false)) > >> + goto enomem; > >> + > > > > This call might linearize the packet. > > > > @nonlinear variable might be wrong after this point. > > > >> if (nonlinear) { > >> /* Just create a S/G fd based on the skb */ > >> err = skb_to_sg_fd(priv, skb, &fd); > >> -- > >> 2.35.1.1320.gc452695387.dirty > >> > > > > Perhaps this instead ? > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > index cfe6b57b1da0e45613ac1bbf32ddd6ace329f4fd..5763d2f1bf8dd31b80fda0681361514dad1dc307 > > 100644 > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > > @@ -2272,12 +2272,12 @@ static netdev_tx_t > > dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) > > { > > const int queue_mapping = skb_get_queue_mapping(skb); > > - bool nonlinear = skb_is_nonlinear(skb); > > struct rtnl_link_stats64 *percpu_stats; > > struct dpaa_percpu_priv *percpu_priv; > > struct netdev_queue *txq; > > struct dpaa_priv *priv; > > struct qm_fd fd; > > + bool nonlinear; > > int offset = 0; > > int err = 0; > > > > @@ -2287,6 +2287,10 @@ dpaa_start_xmit(struct sk_buff *skb, struct > > net_device *net_dev) > > > > qm_fd_clear_fd(&fd); > > > > + if (__skb_put_padto(skb, ETH_ZLEN, false)) > > + goto enomem; > > + > > + nonlinear = skb_is_nonlinear(skb); > > if (!nonlinear) { > > /* We're going to store the skb backpointer at the beginning > > * of the data buffer, so we need a privately owned skb > > Thanks for the suggestion; I was having a hard time figuring out where > to call this. > > Do you have any hints for how to test this for correctness? I'm not sure > how to generate a non-linear packet under 60 bytes. I think pktgen can do this, with its frags parameter.
On 9/9/24 13:14, Eric Dumazet wrote: > On Mon, Sep 9, 2024 at 7:07 PM Sean Anderson <sean.anderson@linux.dev> wrote: >> >> On 9/9/24 12:46, Eric Dumazet wrote: >> > On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@linux.dev> wrote: >> >> >> >> When sending packets under 60 bytes, up to three bytes of the buffer following >> >> the data may be leaked. Avoid this by extending all packets to ETH_ZLEN, >> >> ensuring nothing is leaked in the padding. This bug can be reproduced by >> >> running >> >> >> >> $ ping -s 11 destination >> >> >> >> Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet") >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> >> --- >> >> >> >> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> >> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> >> index cfe6b57b1da0..e4e8ee8b7356 100644 >> >> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> >> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> >> @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) >> >> } >> >> #endif >> >> >> >> + /* Packet data is always read as 32-bit words, so zero out any part of >> >> + * the skb which might be sent if we have to pad the packet >> >> + */ >> >> + if (__skb_put_padto(skb, ETH_ZLEN, false)) >> >> + goto enomem; >> >> + >> > >> > This call might linearize the packet. >> > >> > @nonlinear variable might be wrong after this point. >> > >> >> if (nonlinear) { >> >> /* Just create a S/G fd based on the skb */ >> >> err = skb_to_sg_fd(priv, skb, &fd); >> >> -- >> >> 2.35.1.1320.gc452695387.dirty >> >> >> > >> > Perhaps this instead ? >> > >> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> > index cfe6b57b1da0e45613ac1bbf32ddd6ace329f4fd..5763d2f1bf8dd31b80fda0681361514dad1dc307 >> > 100644 >> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c >> > @@ -2272,12 +2272,12 @@ static netdev_tx_t >> > dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) >> > { >> > const int queue_mapping = skb_get_queue_mapping(skb); >> > - bool nonlinear = skb_is_nonlinear(skb); >> > struct rtnl_link_stats64 *percpu_stats; >> > struct dpaa_percpu_priv *percpu_priv; >> > struct netdev_queue *txq; >> > struct dpaa_priv *priv; >> > struct qm_fd fd; >> > + bool nonlinear; >> > int offset = 0; >> > int err = 0; >> > >> > @@ -2287,6 +2287,10 @@ dpaa_start_xmit(struct sk_buff *skb, struct >> > net_device *net_dev) >> > >> > qm_fd_clear_fd(&fd); >> > >> > + if (__skb_put_padto(skb, ETH_ZLEN, false)) >> > + goto enomem; >> > + >> > + nonlinear = skb_is_nonlinear(skb); >> > if (!nonlinear) { >> > /* We're going to store the skb backpointer at the beginning >> > * of the data buffer, so we need a privately owned skb >> >> Thanks for the suggestion; I was having a hard time figuring out where >> to call this. >> >> Do you have any hints for how to test this for correctness? I'm not sure >> how to generate a non-linear packet under 60 bytes. > > I think pktgen can do this, with its frags parameter. OK, I tested both and was able to use ./pktgen/pktgen_sample01_simple.sh -i net5 -m 7e:de:97:38:53:b9 -d 10.0.0.2 -n 3 -s 59 with a call to `pg_set $DEV "frags 2"` added manually. This results in the following result OK: 109(c13+d95) usec, 1 (59byte,0frags) The original patch causes the nonlinear path to be taken (see with the "tx S/G [TOTAL]" statistic) while your suggestion uses the linear path. Both work, since there's no problem using the nonlinear path with a linear skb. --Sen
On Mon, Sep 9, 2024 at 8:02 PM Sean Anderson <sean.anderson@linux.dev> wrote: > OK, I tested both and was able to use > > ./pktgen/pktgen_sample01_simple.sh -i net5 -m 7e:de:97:38:53:b9 -d 10.0.0.2 -n 3 -s 59 > > with a call to `pg_set $DEV "frags 2"` added manually. > > This results in the following result > > OK: 109(c13+d95) usec, 1 (59byte,0frags) > > The original patch causes the nonlinear path to be taken (see with the > "tx S/G [TOTAL]" statistic) while your suggestion uses the linear path. > Both work, since there's no problem using the nonlinear path with a > linear skb. Maybe it works today, but this allocates an extra page for nothing, this is an extra burden. Vast majority of skb_padto() users call it early in ndo_start_xmit(), let's follow this pattern.
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index cfe6b57b1da0..e4e8ee8b7356 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev) } #endif + /* Packet data is always read as 32-bit words, so zero out any part of + * the skb which might be sent if we have to pad the packet + */ + if (__skb_put_padto(skb, ETH_ZLEN, false)) + goto enomem; + if (nonlinear) { /* Just create a S/G fd based on the skb */ err = skb_to_sg_fd(priv, skb, &fd);
When sending packets under 60 bytes, up to three bytes of the buffer following the data may be leaked. Avoid this by extending all packets to ETH_ZLEN, ensuring nothing is leaked in the padding. This bug can be reproduced by running $ ping -s 11 destination Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet") Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++ 1 file changed, 6 insertions(+)