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

[Sean Anderson]

On 9/9/24 13:14, Eric Dumazet wrote:
> On Mon, Sep 9, 2024 at 7:07 PM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
>>
>> On 9/9/24 12:46, Eric Dumazet wrote:
>> > On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@xxxxxxxxx> 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@xxxxxxxxx>
>> >> ---
>> >>
>> >>  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