[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[John Fastabend]

XDP checks to ensure the MTU is valid and LRO is disabled when it is
loaded. But user configuration after XDP is loaded could change these
and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ee20a2b..156357e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6437,6 +6437,19 @@ static void ixgbe_free_all_rx_resources(struct ixgbe_adapter *adapter)
 static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	int i, frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+
+	/* If XDP is running enforce MTU limitations */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		struct ixgbe_ring *ring = adapter->rx_ring[i];
+
+		if (frame_size > ixgbe_rx_bufsz(ring)) {
+			e_warn(probe,
+			       "Setting MTU > %i with XDP is not supported\n",
+			       ixgbe_rx_bufsz(ring));
+			return -EINVAL;
+		}
+	}
 
 	/*
 	 * For 82599EB we cannot allow legacy VFs to enable their receive
@@ -9291,6 +9304,10 @@ static netdev_features_t ixgbe_fix_features(struct net_device *netdev,
 	if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE))
 		features &= ~NETIF_F_LRO;
 
+	/* If XDP is enabled we can not enable LRO */
+	if (adapter->xdp_prog)
+		features &= ~NETIF_F_LRO;
+
 	return features;
 }
 

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[Singh, Krishneil K]

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of John Fastabend
Sent: Monday, May 1, 2017 10:07 PM
To: intel-wired-lan@lists.osuosl.org
Subject: [Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

With this patch applied, we are seeing issue where setting initial MTU >= 1515 fails with Error: Setting MTU > 1536 with XDP is not supported, but setting MTU to 1514  , we can now set MTU till 3050. If we set anything greater than 3050 it errors out with the following message: Setting MTU > 3072 with XDP is not supported.

Is there a way to see if XDP is enabled and is XDP is supposed to be enabled by default on ixgbe ? only info about XDP seen on platform is in dmesg as follows. 

[    3.267909] ixgbe 0000:04:00.0: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0
[    3.724686] ixgbe 0000:04:00.1: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0
[    4.186271] ixgbe 0000:06:00.0: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0
[    4.644388] ixgbe 0000:06:00.1: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0

When using latest iproute2 tool to disable xdp ( ip link set ethX xdp off) , there is no error reported by iproute2 and nothing is reported in dmesg but we were still unable to set MTU > = 3051.

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[Alexander Duyck]

On Mon, May 1, 2017 at 10:06 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> XDP checks to ensure the MTU is valid and LRO is disabled when it is
> loaded. But user configuration after XDP is loaded could change these
> and cause a misconfiguration.
>
> This patch adds checks to ensure config changes are valid.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ee20a2b..156357e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -6437,6 +6437,19 @@ static void ixgbe_free_all_rx_resources(struct ixgbe_adapter *adapter)
>  static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
>  {
>         struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +       int i, frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +
> +       /* If XDP is running enforce MTU limitations */
> +       for (i = 0; i < adapter->num_rx_queues; i++) {
> +               struct ixgbe_ring *ring = adapter->rx_ring[i];
> +
> +               if (frame_size > ixgbe_rx_bufsz(ring)) {
> +                       e_warn(probe,
> +                              "Setting MTU > %i with XDP is not supported\n",
> +                              ixgbe_rx_bufsz(ring));
> +                       return -EINVAL;
> +               }
> +       }
>

There are a few issues with this piece.

First the Rx buffer size gets updated depending on if we are using
jumbo frames or not. We either use 1.5K or 3K to hold the contents of
the frame. Instead of using ixgbe_rx_bufsz you might just make it so
that you use IXGBE_MAX_FRAME_BUILD_SKB if the adapter flag for
LEGACY_RX is not set, otherwise you can probably just use
IXGBE_RXBUFFER_2K.

Also you should only be performing this check if xdp_prog is set and
it doesn't look like you are checking for that.

>         /*
>          * For 82599EB we cannot allow legacy VFs to enable their receive
> @@ -9291,6 +9304,10 @@ static netdev_features_t ixgbe_fix_features(struct net_device *netdev,
>         if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE))
>                 features &= ~NETIF_F_LRO;
>
> +       /* If XDP is enabled we can not enable LRO */
> +       if (adapter->xdp_prog)
> +               features &= ~NETIF_F_LRO;
> +
>         return features;
>  }
>
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of John Fastabend
> Sent: Monday, May 1, 2017 10:07 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration
> changes while XDP is loaded
> 
> XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded.
> But user configuration after XDP is loaded could change these and cause a
> misconfiguration.
> 
> This patch adds checks to ensure config changes are valid.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[Jeff Kirsher]

On Mon, 2017-05-01 at 22:06 -0700, John Fastabend wrote:
> XDP checks to ensure the MTU is valid and LRO is disabled when it is
> loaded. But user configuration after XDP is loaded could change these
> and cause a misconfiguration.
> 
> This patch adds checks to ensure config changes are valid.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17
> +++++++++++++++++
>  1 file changed, 17 insertions(+)

I know you have moved onto a new job, which is probably taking up most
of your time.  I just need to know if you will be re-spinning this
patch based on the feedback from Alex.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170606/8dd16407/attachment.asc>

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[John Fastabend]

On 06/06/2017 01:14 PM, Jeff Kirsher wrote:
> On Mon, 2017-05-01 at 22:06 -0700, John Fastabend wrote:
>> XDP checks to ensure the MTU is valid and LRO is disabled when it is
>> loaded. But user configuration after XDP is loaded could change these
>> and cause a misconfiguration.
>>
>> This patch adds checks to ensure config changes are valid.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17
>> +++++++++++++++++
>>  1 file changed, 17 insertions(+)
> 
> I know you have moved onto a new job, which is probably taking up most
> of your time.  I just need to know if you will be re-spinning this
> patch based on the feedback from Alex.
> 

I planned to get the xdp_redirect patches updated and sent out
in the next few weeks. When I get around to that I'll also push
an update to this assuming no one beats me to it.

Thanks,
John

[Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

[Jeff Kirsher]

On Tue, 2017-06-06 at 13:17 -0700, John Fastabend wrote:
> On 06/06/2017 01:14 PM, Jeff Kirsher wrote:
> > On Mon, 2017-05-01 at 22:06 -0700, John Fastabend wrote:
> > > XDP checks to ensure the MTU is valid and LRO is disabled when it
> > > is
> > > loaded. But user configuration after XDP is loaded could change
> > > these
> > > and cause a misconfiguration.
> > > 
> > > This patch adds checks to ensure config changes are valid.
> > > 
> > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17
> > > +++++++++++++++++
> > >   1 file changed, 17 insertions(+)
> > 
> > I know you have moved onto a new job, which is probably taking up
> > most
> > of your time.  I just need to know if you will be re-spinning this
> > patch based on the feedback from Alex.
> > 
> 
> I planned to get the xdp_redirect patches updated and sent out
> in the next few weeks. When I get around to that I'll also push
> an update to this assuming no one beats me to it.

Ok thanks for the update, I will drop the current patch and will await
an updated version.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170606/1d7b3c21/attachment.asc>