* [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
@ 2024-09-09 16:06 Sean Anderson
2024-09-09 16:46 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Sean Anderson @ 2024-09-09 16:06 UTC (permalink / raw)
To: Madalin Bucur, Eric Dumazet, netdev
Cc: Paolo Abeni, Jakub Kicinski, linux-kernel, David S . Miller,
Sean Anderson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
2024-09-09 16:06 [PATCH net] net: dpaa: Pad packets to ETH_ZLEN Sean Anderson
@ 2024-09-09 16:46 ` Eric Dumazet
2024-09-09 17:06 ` Sean Anderson
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2024-09-09 16:46 UTC (permalink / raw)
To: Sean Anderson
Cc: Madalin Bucur, netdev, Paolo Abeni, Jakub Kicinski, linux-kernel,
David S . Miller
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
2024-09-09 16:46 ` Eric Dumazet
@ 2024-09-09 17:06 ` Sean Anderson
2024-09-09 17:14 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Sean Anderson @ 2024-09-09 17:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: Madalin Bucur, netdev, Paolo Abeni, Jakub Kicinski, linux-kernel,
David S . Miller
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
2024-09-09 17:06 ` Sean Anderson
@ 2024-09-09 17:14 ` Eric Dumazet
2024-09-09 18:02 ` Sean Anderson
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2024-09-09 17:14 UTC (permalink / raw)
To: Sean Anderson
Cc: Madalin Bucur, netdev, Paolo Abeni, Jakub Kicinski, linux-kernel,
David S . Miller
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
2024-09-09 17:14 ` Eric Dumazet
@ 2024-09-09 18:02 ` Sean Anderson
2024-09-10 8:56 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Sean Anderson @ 2024-09-09 18:02 UTC (permalink / raw)
To: Eric Dumazet
Cc: Madalin Bucur, netdev, Paolo Abeni, Jakub Kicinski, linux-kernel,
David S . Miller
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN
2024-09-09 18:02 ` Sean Anderson
@ 2024-09-10 8:56 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2024-09-10 8:56 UTC (permalink / raw)
To: Sean Anderson
Cc: Madalin Bucur, netdev, Paolo Abeni, Jakub Kicinski, linux-kernel,
David S . Miller
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-10 8:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-09 16:06 [PATCH net] net: dpaa: Pad packets to ETH_ZLEN Sean Anderson
2024-09-09 16:46 ` Eric Dumazet
2024-09-09 17:06 ` Sean Anderson
2024-09-09 17:14 ` Eric Dumazet
2024-09-09 18:02 ` Sean Anderson
2024-09-10 8:56 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).