1. Patchew
  2. linux
  3. View series

[PATCH net] net: dpaa: Pad packets to ETH_ZLEN

Sean Anderson posted 1 patch 2 months, 3 weeks ago
There is a newer version of this series
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++
1 file changed, 6 insertions(+)
[PATCH net] net: dpaa: Pad packets to ETH_ZLEN
Posted by Sean Anderson 2 months, 3 weeks ago 
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;
+
 	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
Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
Posted by Eric Dumazet 2 months, 3 weeks ago 
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
Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
Posted by Sean Anderson 2 months, 3 weeks ago 
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
Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
Posted by Eric Dumazet 2 months, 3 weeks ago 
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.
Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
Posted by Sean Anderson 2 months, 3 weeks ago 
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
Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
Posted by Eric Dumazet 2 months, 3 weeks ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.