| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 30 May 2018 14:55:37 -0700
From: Song Liu <liu.song.a23@...il.com>
To: Jesper Dangaard Brouer <brouer@...hat.com>
Cc: Networking <netdev@...r.kernel.org>,
Daniel Borkmann <borkmann@...earbox.net>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
John Fastabend <john.fastabend@...il.com>,
makita.toshiaki@....ntt.co.jp
Subject: Re: [bpf-next V1 PATCH 1/8] xdp: add flags argument to ndo_xdp_xmit API
On Wed, May 30, 2018 at 11:00 AM, Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
> This patch only change the API and reject any use of flags. This is an
> intermediate step that allows us to implement the flush flag operation
> later, for each individual driver in a separate patch.
>
> The plan is to implement flush operation via XDP_XMIT_FLUSH flag
> and then remove XDP_XMIT_FLAGS_NONE when done.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 6 +++++-
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 3 ++-
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++-
> drivers/net/tun.c | 8 ++++++--
> drivers/net/virtio_net.c | 5 ++++-
> include/linux/netdevice.h | 7 ++++---
> include/net/xdp.h | 5 +++++
> kernel/bpf/devmap.c | 2 +-
> net/core/filter.c | 2 +-
> 9 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 9b698c5acd05..c0451d6e0790 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -3670,7 +3670,8 @@ netdev_tx_t i40e_lan_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
> * For error cases, a negative errno code is returned and no-frames
> * are transmitted (caller must handle freeing frames).
> **/
> -int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
> +int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
> + u32 flags)
> {
> struct i40e_netdev_priv *np = netdev_priv(dev);
> unsigned int queue_index = smp_processor_id();
> @@ -3684,6 +3685,9 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
> if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
> return -ENXIO;
>
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> + return -EINVAL;
> +
> for (i = 0; i < n; i++) {
> struct xdp_frame *xdpf = frames[i];
> int err;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index eb8804b3d7b6..820f76db251b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -487,7 +487,8 @@ u32 i40e_get_tx_pending(struct i40e_ring *ring, bool in_sw);
> void i40e_detect_recover_hung(struct i40e_vsi *vsi);
> int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size);
> bool __i40e_chk_linearize(struct sk_buff *skb);
> -int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames);
> +int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
> + u32 flags);
> void i40e_xdp_flush(struct net_device *dev);
>
> /**
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 031d65c4178d..87f088f4af52 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -10023,7 +10023,7 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> }
>
> static int ixgbe_xdp_xmit(struct net_device *dev, int n,
> - struct xdp_frame **frames)
> + struct xdp_frame **frames, u32 flags)
> {
> struct ixgbe_adapter *adapter = netdev_priv(dev);
> struct ixgbe_ring *ring;
> @@ -10033,6 +10033,9 @@ static int ixgbe_xdp_xmit(struct net_device *dev, int n,
> if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
> return -ENETDOWN;
>
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> + return -EINVAL;
> +
> /* During program transitions its possible adapter->xdp_prog is assigned
> * but ring has not been configured yet. In this case simply abort xmit.
> */
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 2265d2ccea47..b182b8cdd219 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1285,7 +1285,8 @@ static const struct net_device_ops tun_netdev_ops = {
> .ndo_get_stats64 = tun_net_get_stats64,
> };
>
> -static int tun_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
> +static int tun_xdp_xmit(struct net_device *dev, int n,
> + struct xdp_frame **frames, u32 flags)
> {
> struct tun_struct *tun = netdev_priv(dev);
> struct tun_file *tfile;
> @@ -1294,6 +1295,9 @@ static int tun_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames
> int cnt = n;
> int i;
>
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> + return -EINVAL;
> +
> rcu_read_lock();
>
> numqueues = READ_ONCE(tun->numqueues);
> @@ -1332,7 +1336,7 @@ static int tun_xdp_tx(struct net_device *dev, struct xdp_buff *xdp)
> if (unlikely(!frame))
> return -EOVERFLOW;
>
> - return tun_xdp_xmit(dev, 1, &frame);
> + return tun_xdp_xmit(dev, 1, &frame, 0);
> }
>
> static void tun_xdp_flush(struct net_device *dev)
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b2647dd5d302..4ed823625953 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -468,7 +468,7 @@ static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
> }
>
> static int virtnet_xdp_xmit(struct net_device *dev,
> - int n, struct xdp_frame **frames)
> + int n, struct xdp_frame **frames, u32 flags)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> struct receive_queue *rq = vi->rq;
> @@ -481,6 +481,9 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> int err;
> int i;
>
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> + return -EINVAL;
> +
> qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
> sq = &vi->sq[qp];
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8452f72087ef..7f17785a59d7 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1185,13 +1185,13 @@ struct dev_ifalias {
> * This function is used to set or query state related to XDP on the
> * netdevice and manage BPF offload. See definition of
> * enum bpf_netdev_command for details.
> - * int (*ndo_xdp_xmit)(struct net_device *dev, int n, struct xdp_frame **xdp);
> + * int (*ndo_xdp_xmit)(struct net_device *dev, int n, struct xdp_frame **xdp,
> + * u32 flags);
> * This function is used to submit @n XDP packets for transmit on a
> * netdevice. Returns number of frames successfully transmitted, frames
> * that got dropped are freed/returned via xdp_return_frame().
> * Returns negative number, means general error invoking ndo, meaning
> * no frames were xmit'ed and core-caller will free all frames.
> - * TODO: Consider add flag to allow sending flush operation.
> * void (*ndo_xdp_flush)(struct net_device *dev);
> * This function is used to inform the driver to flush a particular
> * xdp tx queue. Must be called on same CPU as xdp_xmit.
> @@ -1380,7 +1380,8 @@ struct net_device_ops {
> int (*ndo_bpf)(struct net_device *dev,
> struct netdev_bpf *bpf);
> int (*ndo_xdp_xmit)(struct net_device *dev, int n,
> - struct xdp_frame **xdp);
> + struct xdp_frame **xdp,
> + u32 flags);
> void (*ndo_xdp_flush)(struct net_device *dev);
> };
>
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 7ad779237ae8..308a4b30b484 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -40,6 +40,11 @@ enum xdp_mem_type {
> MEM_TYPE_MAX,
> };
>
> +/* XDP flags for ndo_xdp_xmit */
> +#define XDP_XMIT_FLAGS_NONE 0U
> +#define XDP_XMIT_FLUSH (1U << 0)
> +#define XDP_XMIT_FLAGS_MASK XDP_XMIT_FLUSH
> +
I guess we need more documentation here on what XDP_XMIT_FLUSH does.
Other than this, it looks good to me.
Acked-by: Song Liu <songliubraving@...com>
> struct xdp_mem_info {
> u32 type; /* enum xdp_mem_type, but known size type */
> u32 id;
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index ae16d0c373ef..04fbd75a5274 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -232,7 +232,7 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
> prefetch(xdpf);
> }
>
> - sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q);
> + sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, 0);
> if (sent < 0) {
> err = sent;
> sent = 0;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 81bd2e9fe8fc..6a21dbcad350 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3056,7 +3056,7 @@ static int __bpf_tx_xdp(struct net_device *dev,
> if (unlikely(!xdpf))
> return -EOVERFLOW;
>
> - sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf);
> + sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf, 0);
> if (sent <= 0)
> return sent;
> dev->netdev_ops->ndo_xdp_flush(dev);
>
Powered by blists - more mailing lists