[PATCH net-next 0/3] xsk: improvements to RX queue check and replace

[PATCH net-next 0/3] xsk: improvements to RX queue check and replace

[Jakub Kicinski]

Hi!

First 3 patches of my recent RFC.  The first one make the check against
real_num_rx_queues slightly more reliable, while the latter two redefine
XDP_QUERY_XSK_UMEM slightly to disallow replacing UMEM in the driver at
the stack level.

I'm not sure where this lays on the bpf vs net trees scale, but there
should be no conflicts with either tree.

Jakub Kicinski (3):
  net: update real_num_rx_queues even when !CONFIG_SYSFS
  xsk: refactor xdp_umem_assign_dev()
  xsk: don't allow umem replace at stack level

 include/linux/netdevice.h | 10 +++---
 net/xdp/xdp_umem.c        | 70 +++++++++++++++++++++++----------------
 2 files changed, 47 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] net: update real_num_rx_queues even when !CONFIG_SYSFS

[Jakub Kicinski]

We used to depend on real_num_rx_queues as a upper bound for sanity
checks.  For AF_XDP socket validation it's useful if the check behaves
the same regardless of CONFIG_SYSFS setting.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 include/linux/netdevice.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9c917467a2c7..3bf7e93c9e96 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3431,8 +3431,9 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq);
 int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq);
 #else
 static inline int netif_set_real_num_rx_queues(struct net_device *dev,
-						unsigned int rxq)
+						unsigned int rxqs)
 {
+	dev->real_num_rx_queues = rxqs;
 	return 0;
 }
 #endif
-- 
2.17.1

Re: [PATCH net-next 1/3] net: update real_num_rx_queues even when !CONFIG_SYSFS

[Jesper Dangaard Brouer]

On Mon, 30 Jul 2018 20:43:51 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> We used to depend on real_num_rx_queues as a upper bound for sanity
> checks.  For AF_XDP socket validation it's useful if the check behaves
> the same regardless of CONFIG_SYSFS setting.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

> ---
>  include/linux/netdevice.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 9c917467a2c7..3bf7e93c9e96 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3431,8 +3431,9 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq);
>  int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq);
>  #else
>  static inline int netif_set_real_num_rx_queues(struct net_device *dev,
> -						unsigned int rxq)
> +						unsigned int rxqs)
>  {
> +	dev->real_num_rx_queues = rxqs;
>  	return 0;
>  }
>  #endif

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

[PATCH net-next 2/3] xsk: refactor xdp_umem_assign_dev()

[Jakub Kicinski]

Return early and only take the ref on dev once there is no possibility
of failing.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Björn Töpel <bjorn.topel@gmail.com>
---
 net/xdp/xdp_umem.c | 49 ++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index f47abb46c587..c199d66b5f3f 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -56,41 +56,34 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 	if (force_copy)
 		return 0;
 
-	dev_hold(dev);
+	if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
+		return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
 
-	if (dev->netdev_ops->ndo_bpf && dev->netdev_ops->ndo_xsk_async_xmit) {
-		bpf.command = XDP_QUERY_XSK_UMEM;
+	bpf.command = XDP_QUERY_XSK_UMEM;
 
-		rtnl_lock();
-		err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-		rtnl_unlock();
+	rtnl_lock();
+	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
+	rtnl_unlock();
 
-		if (err) {
-			dev_put(dev);
-			return force_zc ? -ENOTSUPP : 0;
-		}
+	if (err)
+		return force_zc ? -ENOTSUPP : 0;
 
-		bpf.command = XDP_SETUP_XSK_UMEM;
-		bpf.xsk.umem = umem;
-		bpf.xsk.queue_id = queue_id;
+	bpf.command = XDP_SETUP_XSK_UMEM;
+	bpf.xsk.umem = umem;
+	bpf.xsk.queue_id = queue_id;
 
-		rtnl_lock();
-		err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-		rtnl_unlock();
+	rtnl_lock();
+	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
+	rtnl_unlock();
 
-		if (err) {
-			dev_put(dev);
-			return force_zc ? err : 0; /* fail or fallback */
-		}
-
-		umem->dev = dev;
-		umem->queue_id = queue_id;
-		umem->zc = true;
-		return 0;
-	}
+	if (err)
+		return force_zc ? err : 0; /* fail or fallback */
 
-	dev_put(dev);
-	return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
+	dev_hold(dev);
+	umem->dev = dev;
+	umem->queue_id = queue_id;
+	umem->zc = true;
+	return 0;
 }
 
 static void xdp_umem_clear_dev(struct xdp_umem *umem)
-- 
2.17.1

[PATCH net-next 3/3] xsk: don't allow umem replace at stack level

[Jakub Kicinski]

Currently drivers have to check if they already have a umem
installed for a given queue and return an error if so.  Make
better use of XDP_QUERY_XSK_UMEM and move this functionality
to the core.

We need to keep rtnl across the calls now.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Björn Töpel <bjorn.topel@gmail.com>
---
 include/linux/netdevice.h |  7 ++++---
 net/xdp/xdp_umem.c        | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3bf7e93c9e96..282e2e95ad5b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -872,10 +872,10 @@ struct netdev_bpf {
 		struct {
 			struct bpf_offloaded_map *offmap;
 		};
-		/* XDP_SETUP_XSK_UMEM */
+		/* XDP_QUERY_XSK_UMEM, XDP_SETUP_XSK_UMEM */
 		struct {
-			struct xdp_umem *umem;
-			u16 queue_id;
+			struct xdp_umem *umem; /* out for query*/
+			u16 queue_id; /* in for query */
 		} xsk;
 	};
 };
@@ -3568,6 +3568,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		      int fd, u32 flags);
 u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
 		    enum bpf_netdev_command cmd);
+int xdp_umem_query(struct net_device *dev, u16 queue_id);
 
 int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
 int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index c199d66b5f3f..911ca6d3cb5a 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -11,6 +11,8 @@
 #include <linux/slab.h>
 #include <linux/bpf.h>
 #include <linux/mm.h>
+#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
 
 #include "xdp_umem.h"
 #include "xsk_queue.h"
@@ -40,6 +42,21 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
 	}
 }
 
+int xdp_umem_query(struct net_device *dev, u16 queue_id)
+{
+	struct netdev_bpf bpf;
+
+	ASSERT_RTNL();
+
+	memset(&bpf, 0, sizeof(bpf));
+	bpf.command = XDP_QUERY_XSK_UMEM;
+	bpf.xsk.queue_id = queue_id;
+
+	if (!dev->netdev_ops->ndo_bpf)
+		return 0;
+	return dev->netdev_ops->ndo_bpf(dev, &bpf) ?: !!bpf.xsk.umem;
+}
+
 int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 			u32 queue_id, u16 flags)
 {
@@ -62,28 +79,30 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 	bpf.command = XDP_QUERY_XSK_UMEM;
 
 	rtnl_lock();
-	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-	rtnl_unlock();
-
-	if (err)
-		return force_zc ? -ENOTSUPP : 0;
+	err = xdp_umem_query(dev, queue_id);
+	if (err) {
+		err = err < 0 ? -ENOTSUPP : -EBUSY;
+		goto err_rtnl_unlock;
+	}
 
 	bpf.command = XDP_SETUP_XSK_UMEM;
 	bpf.xsk.umem = umem;
 	bpf.xsk.queue_id = queue_id;
 
-	rtnl_lock();
 	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-	rtnl_unlock();
-
 	if (err)
-		return force_zc ? err : 0; /* fail or fallback */
+		goto err_rtnl_unlock;
+	rtnl_unlock();
 
 	dev_hold(dev);
 	umem->dev = dev;
 	umem->queue_id = queue_id;
 	umem->zc = true;
 	return 0;
+
+err_rtnl_unlock:
+	rtnl_unlock();
+	return force_zc ? err : 0; /* fail or fallback */
 }
 
 static void xdp_umem_clear_dev(struct xdp_umem *umem)
-- 
2.17.1

Re: [PATCH net-next 0/3] xsk: improvements to RX queue check and replace

[Björn Töpel]

Den tis 31 juli 2018 kl 05:46 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> Hi!
>
> First 3 patches of my recent RFC.  The first one make the check against
> real_num_rx_queues slightly more reliable, while the latter two redefine
> XDP_QUERY_XSK_UMEM slightly to disallow replacing UMEM in the driver at
> the stack level.
>
> I'm not sure where this lays on the bpf vs net trees scale, but there
> should be no conflicts with either tree.
>
> Jakub Kicinski (3):
>   net: update real_num_rx_queues even when !CONFIG_SYSFS
>   xsk: refactor xdp_umem_assign_dev()
>   xsk: don't allow umem replace at stack level
>
>  include/linux/netdevice.h | 10 +++---
>  net/xdp/xdp_umem.c        | 70 +++++++++++++++++++++++----------------
>  2 files changed, 47 insertions(+), 33 deletions(-)
>
> --
> 2.17.1
>

LGTM!

For the series:
Acked-by: Björn Töpel <bjorn.topel@intel.com>

Re: [PATCH net-next 0/3] xsk: improvements to RX queue check and replace

[Daniel Borkmann]

On 07/31/2018 05:43 AM, Jakub Kicinski wrote:
> Hi!
> 
> First 3 patches of my recent RFC.  The first one make the check against
> real_num_rx_queues slightly more reliable, while the latter two redefine
> XDP_QUERY_XSK_UMEM slightly to disallow replacing UMEM in the driver at
> the stack level.
> 
> I'm not sure where this lays on the bpf vs net trees scale, but there
> should be no conflicts with either tree.

I'm fine either way, in any case, series looks good to me:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

> Jakub Kicinski (3):
>   net: update real_num_rx_queues even when !CONFIG_SYSFS
>   xsk: refactor xdp_umem_assign_dev()
>   xsk: don't allow umem replace at stack level
> 
>  include/linux/netdevice.h | 10 +++---
>  net/xdp/xdp_umem.c        | 70 +++++++++++++++++++++++----------------
>  2 files changed, 47 insertions(+), 33 deletions(-)
> 

Re: [PATCH net-next 0/3] xsk: improvements to RX queue check and replace

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue, 31 Jul 2018 09:50:12 +0200

> On 07/31/2018 05:43 AM, Jakub Kicinski wrote:
>> Hi!
>> 
>> First 3 patches of my recent RFC.  The first one make the check against
>> real_num_rx_queues slightly more reliable, while the latter two redefine
>> XDP_QUERY_XSK_UMEM slightly to disallow replacing UMEM in the driver at
>> the stack level.
>> 
>> I'm not sure where this lays on the bpf vs net trees scale, but there
>> should be no conflicts with either tree.
> 
> I'm fine either way, in any case, series looks good to me:
> 
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Series applied to net-next then :)