All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-02 23:11 ` Matteo Croce
  0 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-04-02 23:11 UTC (permalink / raw)
  To: netdev; +Cc: Sunil Goutham, Robert Richter, linux-arm-kernel

The thunderx driver forbids to load an eBPF program if the MTU is higher
than 1500 bytes, but this can be circumvented by first loading the eBPF,
and then raising the MTU.

XDP assumes that SKBs are linear and fit in a single page, this can lead
to undefined behaviours.
Fix this by limiting the MTU to 1500 bytes if an eBPF program is loaded.

Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index aa2be4807191..fe1d7513f01d 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1578,6 +1578,13 @@ static int nicvf_change_mtu(struct net_device *netdev, int new_mtu)
 	struct nicvf *nic = netdev_priv(netdev);
 	int orig_mtu = netdev->mtu;
 
+	/* For now just support only the usual MTU sized frames */
+	if (nic->xdp_prog && new_mtu > 1500) {
+		netdev_warn(netdev, "Jumbo frames not yet supported with XDP, current MTU %d.\n",
+			    netdev->mtu);
+		return -EOPNOTSUPP;
+	}
+
 	netdev->mtu = new_mtu;
 
 	if (!netif_running(netdev))
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-02 23:11 ` Matteo Croce
  0 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-04-02 23:11 UTC (permalink / raw)
  To: netdev; +Cc: Sunil Goutham, Robert Richter, linux-arm-kernel

The thunderx driver forbids to load an eBPF program if the MTU is higher
than 1500 bytes, but this can be circumvented by first loading the eBPF,
and then raising the MTU.

XDP assumes that SKBs are linear and fit in a single page, this can lead
to undefined behaviours.
Fix this by limiting the MTU to 1500 bytes if an eBPF program is loaded.

Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index aa2be4807191..fe1d7513f01d 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1578,6 +1578,13 @@ static int nicvf_change_mtu(struct net_device *netdev, int new_mtu)
 	struct nicvf *nic = netdev_priv(netdev);
 	int orig_mtu = netdev->mtu;
 
+	/* For now just support only the usual MTU sized frames */
+	if (nic->xdp_prog && new_mtu > 1500) {
+		netdev_warn(netdev, "Jumbo frames not yet supported with XDP, current MTU %d.\n",
+			    netdev->mtu);
+		return -EOPNOTSUPP;
+	}
+
 	netdev->mtu = new_mtu;
 
 	if (!netif_running(netdev))
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
  2019-04-02 23:11 ` Matteo Croce
@ 2019-04-03  7:18   ` Jesper Dangaard Brouer
  -1 siblings, 0 replies; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2019-04-03  7:18 UTC (permalink / raw)
  To: Matteo Croce
  Cc: brouer, netdev, Sunil Goutham, Robert Richter, linux-arm-kernel,
	Ilias Apalodimas

On Wed,  3 Apr 2019 01:11:36 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> The thunderx driver forbids to load an eBPF program if the MTU is higher
> than 1500 bytes, but this can be circumvented by first loading the eBPF,
> and then raising the MTU.
> 
> XDP assumes that SKBs are linear and fit in a single page, this can lead
> to undefined behaviours.
> Fix this by limiting the MTU to 1500 bytes if an eBPF program is loaded.

I find this 1500 bytes limit strange.  While XDP does not support
frames that is split over multiple pages, it does support larger frames,
as long as it can fit within one (e.g 4K) page, minus XDP_PACKET_HEADROOM
(256B) and have tail-room for skb_shared_info (320 bytes), which is
4096-256-320 = 3520 bytes.

A quick look at this driver it seems you are limited to 2176 bytes
(RCV_FRAG_LEN=1536+64+320 + 256) based on how the dma mapping is done.
I think the drivers intent is to limit to 1536 bytes, but the DMA
mapping area include more.  It seems rather suboptimal that the
skb_shared_info is included as part of the DMA mapping.


> Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
>  drivers/net/ethernet/cavium/thunder/nicvf_main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index aa2be4807191..fe1d7513f01d 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1578,6 +1578,13 @@ static int nicvf_change_mtu(struct net_device *netdev, int new_mtu)
>  	struct nicvf *nic = netdev_priv(netdev);
>  	int orig_mtu = netdev->mtu;
>  
> +	/* For now just support only the usual MTU sized frames */
> +	if (nic->xdp_prog && new_mtu > 1500) {
> +		netdev_warn(netdev, "Jumbo frames not yet supported with XDP, current MTU %d.\n",
> +			    netdev->mtu);
> +		return -EOPNOTSUPP;
> +	}
> +
>  	netdev->mtu = new_mtu;
>  
>  	if (!netif_running(netdev))



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-03  7:18   ` Jesper Dangaard Brouer
  0 siblings, 0 replies; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2019-04-03  7:18 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Robert Richter, netdev, Ilias Apalodimas, brouer, Sunil Goutham,
	linux-arm-kernel

On Wed,  3 Apr 2019 01:11:36 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> The thunderx driver forbids to load an eBPF program if the MTU is higher
> than 1500 bytes, but this can be circumvented by first loading the eBPF,
> and then raising the MTU.
> 
> XDP assumes that SKBs are linear and fit in a single page, this can lead
> to undefined behaviours.
> Fix this by limiting the MTU to 1500 bytes if an eBPF program is loaded.

I find this 1500 bytes limit strange.  While XDP does not support
frames that is split over multiple pages, it does support larger frames,
as long as it can fit within one (e.g 4K) page, minus XDP_PACKET_HEADROOM
(256B) and have tail-room for skb_shared_info (320 bytes), which is
4096-256-320 = 3520 bytes.

A quick look at this driver it seems you are limited to 2176 bytes
(RCV_FRAG_LEN=1536+64+320 + 256) based on how the dma mapping is done.
I think the drivers intent is to limit to 1536 bytes, but the DMA
mapping area include more.  It seems rather suboptimal that the
skb_shared_info is included as part of the DMA mapping.


> Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
>  drivers/net/ethernet/cavium/thunder/nicvf_main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index aa2be4807191..fe1d7513f01d 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1578,6 +1578,13 @@ static int nicvf_change_mtu(struct net_device *netdev, int new_mtu)
>  	struct nicvf *nic = netdev_priv(netdev);
>  	int orig_mtu = netdev->mtu;
>  
> +	/* For now just support only the usual MTU sized frames */
> +	if (nic->xdp_prog && new_mtu > 1500) {
> +		netdev_warn(netdev, "Jumbo frames not yet supported with XDP, current MTU %d.\n",
> +			    netdev->mtu);
> +		return -EOPNOTSUPP;
> +	}
> +
>  	netdev->mtu = new_mtu;
>  
>  	if (!netif_running(netdev))



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
  2019-04-02 23:11 ` Matteo Croce
@ 2019-04-05  0:20   ` David Miller
  -1 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2019-04-05  0:20 UTC (permalink / raw)
  To: mcroce; +Cc: netdev, sgoutham, rric, linux-arm-kernel

From: Matteo Croce <mcroce@redhat.com>
Date: Wed,  3 Apr 2019 01:11:36 +0200

> The thunderx driver forbids to load an eBPF program if the MTU is higher
> than 1500 bytes, but this can be circumvented by first loading the eBPF,
> and then raising the MTU.
> 
> XDP assumes that SKBs are linear and fit in a single page, this can lead
> to undefined behaviours.
> Fix this by limiting the MTU to 1500 bytes if an eBPF program is loaded.
> 
> Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Please respond to Jesper's feedback about your choice of a limit of 1500.

Otherwise I will toss your patch.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-05  0:20   ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2019-04-05  0:20 UTC (permalink / raw)
  To: mcroce; +Cc: netdev, sgoutham, rric, linux-arm-kernel

From: Matteo Croce <mcroce@redhat.com>
Date: Wed,  3 Apr 2019 01:11:36 +0200

> The thunderx driver forbids to load an eBPF program if the MTU is higher
> than 1500 bytes, but this can be circumvented by first loading the eBPF,
> and then raising the MTU.
> 
> XDP assumes that SKBs are linear and fit in a single page, this can lead
> to undefined behaviours.
> Fix this by limiting the MTU to 1500 bytes if an eBPF program is loaded.
> 
> Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Please respond to Jesper's feedback about your choice of a limit of 1500.

Otherwise I will toss your patch.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
  2019-04-05  0:20   ` David Miller
@ 2019-04-05  0:51     ` Matteo Croce
  -1 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-04-05  0:51 UTC (permalink / raw)
  To: David Miller, Jesper Dangaard Brouer
  Cc: netdev, Sunil Goutham, Robert Richter, Linux ARM, Ilias Apalodimas

On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:
>
> From: Matteo Croce <mcroce@redhat.com>
> Date: Wed,  3 Apr 2019 01:11:36 +0200
>
> > The thunderx driver forbids to load an eBPF program if the MTU is
> > higher than 1500 bytes, but this can be circumvented by first
> > loading the eBPF, and then raising the MTU.
> >
> > XDP assumes that SKBs are linear and fit in a single page, this can
> > lead to undefined behaviours.
> > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > loaded.
> >
> > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
>
> Please respond to Jesper's feedback about your choice of a limit of
> 1500.
>
> Otherwise I will toss your patch.

Hi David ad Jesper,

I didn't deliberately choose a limit of 1500, the limit is always set
in nicvf_xdp_setup():

    /* For now just support only the usual MTU sized frames */
    if (prog && (dev->mtu > 1500)) {
        netdev_warn(dev, "Jumbo frames not yet supported with XDP...

I just enforced the same limit in another code path which didn't do
the check.
If you think that 1500 is a bad value, and I'm sure you're right because
there isn't room even for VLAN tagging, I will send a series like:
- 1/2 sets the limit to a resonable value
- 2/2 enforce the same limit in the two code paths

Regards,
-- 
Matteo Croce
per aspera ad upstream

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-05  0:51     ` Matteo Croce
  0 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-04-05  0:51 UTC (permalink / raw)
  To: David Miller, Jesper Dangaard Brouer
  Cc: Ilias Apalodimas, netdev, Sunil Goutham, Robert Richter, Linux ARM

On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:
>
> From: Matteo Croce <mcroce@redhat.com>
> Date: Wed,  3 Apr 2019 01:11:36 +0200
>
> > The thunderx driver forbids to load an eBPF program if the MTU is
> > higher than 1500 bytes, but this can be circumvented by first
> > loading the eBPF, and then raising the MTU.
> >
> > XDP assumes that SKBs are linear and fit in a single page, this can
> > lead to undefined behaviours.
> > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > loaded.
> >
> > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
>
> Please respond to Jesper's feedback about your choice of a limit of
> 1500.
>
> Otherwise I will toss your patch.

Hi David ad Jesper,

I didn't deliberately choose a limit of 1500, the limit is always set
in nicvf_xdp_setup():

    /* For now just support only the usual MTU sized frames */
    if (prog && (dev->mtu > 1500)) {
        netdev_warn(dev, "Jumbo frames not yet supported with XDP...

I just enforced the same limit in another code path which didn't do
the check.
If you think that 1500 is a bad value, and I'm sure you're right because
there isn't room even for VLAN tagging, I will send a series like:
- 1/2 sets the limit to a resonable value
- 2/2 enforce the same limit in the two code paths

Regards,
-- 
Matteo Croce
per aspera ad upstream

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
  2019-04-05  0:51     ` Matteo Croce
@ 2019-04-05 15:45       ` Matteo Croce
  -1 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-04-05 15:45 UTC (permalink / raw)
  To: David Miller, Jesper Dangaard Brouer
  Cc: netdev, Sunil Goutham, Robert Richter, Linux ARM, Ilias Apalodimas

On Fri, Apr 5, 2019 at 2:51 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:
> >
> > From: Matteo Croce <mcroce@redhat.com>
> > Date: Wed,  3 Apr 2019 01:11:36 +0200
> >
> > > The thunderx driver forbids to load an eBPF program if the MTU is
> > > higher than 1500 bytes, but this can be circumvented by first
> > > loading the eBPF, and then raising the MTU.
> > >
> > > XDP assumes that SKBs are linear and fit in a single page, this can
> > > lead to undefined behaviours.
> > > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > > loaded.
> > >
> > > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> >
> > Please respond to Jesper's feedback about your choice of a limit of
> > 1500.
> >
> > Otherwise I will toss your patch.
>
> Hi David ad Jesper,
>
> I didn't deliberately choose a limit of 1500, the limit is always set
> in nicvf_xdp_setup():
>
>     /* For now just support only the usual MTU sized frames */
>     if (prog && (dev->mtu > 1500)) {
>         netdev_warn(dev, "Jumbo frames not yet supported with XDP...
>
> I just enforced the same limit in another code path which didn't do
> the check.
> If you think that 1500 is a bad value, and I'm sure you're right because
> there isn't room even for VLAN tagging, I will send a series like:
> - 1/2 sets the limit to a resonable value
> - 2/2 enforce the same limit in the two code paths
>
> Regards,
> --
> Matteo Croce
> per aspera ad upstream

Hi all,

I did some tests and I've found that on this driver, the maximum
allowed frame size with XDP is 1530.
Frames bigger than 1530 are split around multiple pages, so the driver
doesn't even run the bpf on them:

        /* For XDP, ignore pkts spanning multiple pages */
        if (nic->xdp_prog && (cqe_rx->rb_cnt == 1)) {

based on this test, I'll send a series with a proper MTU limit which
should be, correct me if I'm wrong: 1530 - 14 (eth) - 4 (QinQ) = 1512
bytes.
I subtract only the 4 bytes for the QinQ as the
NETIF_F_VLAN_CHALLENGED_BIT flag is not set, and the first VLAN tag
should not be counted.


Regards,
--
Matteo Croce
per aspera ad upstream

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-05 15:45       ` Matteo Croce
  0 siblings, 0 replies; 12+ messages in thread
From: Matteo Croce @ 2019-04-05 15:45 UTC (permalink / raw)
  To: David Miller, Jesper Dangaard Brouer
  Cc: Ilias Apalodimas, netdev, Sunil Goutham, Robert Richter, Linux ARM

On Fri, Apr 5, 2019 at 2:51 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:
> >
> > From: Matteo Croce <mcroce@redhat.com>
> > Date: Wed,  3 Apr 2019 01:11:36 +0200
> >
> > > The thunderx driver forbids to load an eBPF program if the MTU is
> > > higher than 1500 bytes, but this can be circumvented by first
> > > loading the eBPF, and then raising the MTU.
> > >
> > > XDP assumes that SKBs are linear and fit in a single page, this can
> > > lead to undefined behaviours.
> > > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > > loaded.
> > >
> > > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> >
> > Please respond to Jesper's feedback about your choice of a limit of
> > 1500.
> >
> > Otherwise I will toss your patch.
>
> Hi David ad Jesper,
>
> I didn't deliberately choose a limit of 1500, the limit is always set
> in nicvf_xdp_setup():
>
>     /* For now just support only the usual MTU sized frames */
>     if (prog && (dev->mtu > 1500)) {
>         netdev_warn(dev, "Jumbo frames not yet supported with XDP...
>
> I just enforced the same limit in another code path which didn't do
> the check.
> If you think that 1500 is a bad value, and I'm sure you're right because
> there isn't room even for VLAN tagging, I will send a series like:
> - 1/2 sets the limit to a resonable value
> - 2/2 enforce the same limit in the two code paths
>
> Regards,
> --
> Matteo Croce
> per aspera ad upstream

Hi all,

I did some tests and I've found that on this driver, the maximum
allowed frame size with XDP is 1530.
Frames bigger than 1530 are split around multiple pages, so the driver
doesn't even run the bpf on them:

        /* For XDP, ignore pkts spanning multiple pages */
        if (nic->xdp_prog && (cqe_rx->rb_cnt == 1)) {

based on this test, I'll send a series with a proper MTU limit which
should be, correct me if I'm wrong: 1530 - 14 (eth) - 4 (QinQ) = 1512
bytes.
I subtract only the 4 bytes for the QinQ as the
NETIF_F_VLAN_CHALLENGED_BIT flag is not set, and the first VLAN tag
should not be counted.


Regards,
--
Matteo Croce
per aspera ad upstream

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
  2019-04-05 15:45       ` Matteo Croce
@ 2019-04-06 16:19         ` Jesper Dangaard Brouer
  -1 siblings, 0 replies; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2019-04-06 16:19 UTC (permalink / raw)
  To: Matteo Croce
  Cc: David Miller, netdev, Sunil Goutham, Robert Richter, Linux ARM,
	Ilias Apalodimas, brouer

On Fri, 5 Apr 2019 17:45:34 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> On Fri, Apr 5, 2019 at 2:51 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:  
> > >
> > > From: Matteo Croce <mcroce@redhat.com>
> > > Date: Wed,  3 Apr 2019 01:11:36 +0200
> > >  
> > > > The thunderx driver forbids to load an eBPF program if the MTU is
> > > > higher than 1500 bytes, but this can be circumvented by first
> > > > loading the eBPF, and then raising the MTU.
> > > >
> > > > XDP assumes that SKBs are linear and fit in a single page, this can
> > > > lead to undefined behaviours.
> > > > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > > > loaded.
> > > >
> > > > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > > > Signed-off-by: Matteo Croce <mcroce@redhat.com>  
> > >
> > > Please respond to Jesper's feedback about your choice of a limit of
> > > 1500.
> > >
> > > Otherwise I will toss your patch.  
> >
> > Hi David ad Jesper,
> >
> > I didn't deliberately choose a limit of 1500, the limit is always set
> > in nicvf_xdp_setup():
> >
> >     /* For now just support only the usual MTU sized frames */
> >     if (prog && (dev->mtu > 1500)) {
> >         netdev_warn(dev, "Jumbo frames not yet supported with XDP...
> >
> > I just enforced the same limit in another code path which didn't do
> > the check.
> > If you think that 1500 is a bad value, and I'm sure you're right because
> > there isn't room even for VLAN tagging, I will send a series like:
> > - 1/2 sets the limit to a resonable value
> > - 2/2 enforce the same limit in the two code paths
> >
> > Regards,
> > --
> > Matteo Croce
> > per aspera ad upstream  
> 
> Hi all,
> 
> I did some tests and I've found that on this driver, the maximum
> allowed frame size with XDP is 1530.
> Frames bigger than 1530 are split around multiple pages, so the driver
> doesn't even run the bpf on them:
> 
>         /* For XDP, ignore pkts spanning multiple pages */
>         if (nic->xdp_prog && (cqe_rx->rb_cnt == 1)) {
> 
> based on this test, I'll send a series with a proper MTU limit which
> should be, correct me if I'm wrong: 1530 - 14 (eth) - 4 (QinQ) = 1512
> bytes.
> I subtract only the 4 bytes for the QinQ as the
> NETIF_F_VLAN_CHALLENGED_BIT flag is not set, and the first VLAN tag
> should not be counted.

You *do* need to include the first VLAN tag in the calculation.
I guess I didn't explain this clear enough on IRC.

XDP cannot use VLAN-offloading. As we explain here[1] when running XDP,
you need to disable VLAN-offloading (see cmd in [1]), because XDP need
the VLAN header to be "inline" in the packet.  XDP don't (yet) have
access to reading info from the descriptor.

[1] https://github.com/xdp-project/xdp-tutorial/tree/master/packet01-parsing#a-note-about-vlan-offloads
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP
@ 2019-04-06 16:19         ` Jesper Dangaard Brouer
  0 siblings, 0 replies; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2019-04-06 16:19 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Robert Richter, netdev, Ilias Apalodimas, brouer, Sunil Goutham,
	David Miller, Linux ARM

On Fri, 5 Apr 2019 17:45:34 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> On Fri, Apr 5, 2019 at 2:51 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:  
> > >
> > > From: Matteo Croce <mcroce@redhat.com>
> > > Date: Wed,  3 Apr 2019 01:11:36 +0200
> > >  
> > > > The thunderx driver forbids to load an eBPF program if the MTU is
> > > > higher than 1500 bytes, but this can be circumvented by first
> > > > loading the eBPF, and then raising the MTU.
> > > >
> > > > XDP assumes that SKBs are linear and fit in a single page, this can
> > > > lead to undefined behaviours.
> > > > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > > > loaded.
> > > >
> > > > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > > > Signed-off-by: Matteo Croce <mcroce@redhat.com>  
> > >
> > > Please respond to Jesper's feedback about your choice of a limit of
> > > 1500.
> > >
> > > Otherwise I will toss your patch.  
> >
> > Hi David ad Jesper,
> >
> > I didn't deliberately choose a limit of 1500, the limit is always set
> > in nicvf_xdp_setup():
> >
> >     /* For now just support only the usual MTU sized frames */
> >     if (prog && (dev->mtu > 1500)) {
> >         netdev_warn(dev, "Jumbo frames not yet supported with XDP...
> >
> > I just enforced the same limit in another code path which didn't do
> > the check.
> > If you think that 1500 is a bad value, and I'm sure you're right because
> > there isn't room even for VLAN tagging, I will send a series like:
> > - 1/2 sets the limit to a resonable value
> > - 2/2 enforce the same limit in the two code paths
> >
> > Regards,
> > --
> > Matteo Croce
> > per aspera ad upstream  
> 
> Hi all,
> 
> I did some tests and I've found that on this driver, the maximum
> allowed frame size with XDP is 1530.
> Frames bigger than 1530 are split around multiple pages, so the driver
> doesn't even run the bpf on them:
> 
>         /* For XDP, ignore pkts spanning multiple pages */
>         if (nic->xdp_prog && (cqe_rx->rb_cnt == 1)) {
> 
> based on this test, I'll send a series with a proper MTU limit which
> should be, correct me if I'm wrong: 1530 - 14 (eth) - 4 (QinQ) = 1512
> bytes.
> I subtract only the 4 bytes for the QinQ as the
> NETIF_F_VLAN_CHALLENGED_BIT flag is not set, and the first VLAN tag
> should not be counted.

You *do* need to include the first VLAN tag in the calculation.
I guess I didn't explain this clear enough on IRC.

XDP cannot use VLAN-offloading. As we explain here[1] when running XDP,
you need to disable VLAN-offloading (see cmd in [1]), because XDP need
the VLAN header to be "inline" in the packet.  XDP don't (yet) have
access to reading info from the descriptor.

[1] https://github.com/xdp-project/xdp-tutorial/tree/master/packet01-parsing#a-note-about-vlan-offloads
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-04-06 16:19 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-02 23:11 [PATCH net] net: thunderx: don't allow jumbo frames with XDP Matteo Croce
2019-04-02 23:11 ` Matteo Croce
2019-04-03  7:18 ` Jesper Dangaard Brouer
2019-04-03  7:18   ` Jesper Dangaard Brouer
2019-04-05  0:20 ` David Miller
2019-04-05  0:20   ` David Miller
2019-04-05  0:51   ` Matteo Croce
2019-04-05  0:51     ` Matteo Croce
2019-04-05 15:45     ` Matteo Croce
2019-04-05 15:45       ` Matteo Croce
2019-04-06 16:19       ` Jesper Dangaard Brouer
2019-04-06 16:19         ` Jesper Dangaard Brouer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.