netdev - Re: [PATCH net-next v2 2/6] sfc: perform XDP processing on received packets

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191028173321.5254abf3@carbon>
Date:   Mon, 28 Oct 2019 17:33:21 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Charles McLachlan <cmclachlan@...arflare.com>
Cc:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <linux-net-drivers@...arflare.com>, brouer@...hat.com
Subject: Re: [PATCH net-next v2 2/6] sfc: perform XDP processing on received
 packets

On Mon, 28 Oct 2019 13:59:21 +0000
Charles McLachlan <cmclachlan@...arflare.com> wrote:

> diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
> index 85ec07f5a674..6fabb1925ff1 100644
> --- a/drivers/net/ethernet/sfc/rx.c
> +++ b/drivers/net/ethernet/sfc/rx.c
[...]
> @@ -635,6 +642,103 @@ static void efx_rx_deliver(struct efx_channel *channel, u8 *eh,
>  		netif_receive_skb(skb);
>  }
>  
> +/** efx_do_xdp: perform XDP processing on a received packet
> + *
> + * Returns true if packet should still be delivered.
> + */
> +static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,
> +		       struct efx_rx_buffer *rx_buf, u8 **ehp)
> +{
> +	u8 rx_prefix[EFX_MAX_RX_PREFIX_SIZE];
> +	struct efx_rx_queue *rx_queue;
> +	struct bpf_prog *xdp_prog;
> +	struct xdp_buff xdp;
> +	u32 xdp_act;
> +	s16 offset;
> +	int err;
> +
> +	rcu_read_lock();
> +	xdp_prog = rcu_dereference(efx->xdp_prog);
> +	if (!xdp_prog) {
> +		rcu_read_unlock();
> +		return true;
> +	}
> +
> +	rx_queue = efx_channel_get_rx_queue(channel);
> +
> +	if (unlikely(channel->rx_pkt_n_frags > 1)) {
> +		/* We can't do XDP on fragmented packets - drop. */
> +		rcu_read_unlock();
> +		efx_free_rx_buffers(rx_queue, rx_buf,
> +				    channel->rx_pkt_n_frags);
> +		if (net_ratelimit())
> +			netif_err(efx, rx_err, efx->net_dev,
> +				  "XDP is not possible with multiple receive fragments (%d)\n",
> +				  channel->rx_pkt_n_frags);
> +		return false;
> +	}
> +
> +	dma_sync_single_for_cpu(&efx->pci_dev->dev, rx_buf->dma_addr,
> +				rx_buf->len, DMA_FROM_DEVICE);
> +
> +	/* Save the rx prefix. */
> +	EFX_WARN_ON_PARANOID(efx->rx_prefix_size > EFX_MAX_RX_PREFIX_SIZE);
> +	memcpy(rx_prefix, *ehp - efx->rx_prefix_size,
> +	       efx->rx_prefix_size);
> +
> +	xdp.data = *ehp;
> +	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> +
> +	/* No support yet for XDP metadata */
> +	xdp_set_data_meta_invalid(&xdp);
> +	xdp.data_end = xdp.data + rx_buf->len;
> +	xdp.rxq = &rx_queue->xdp_rxq_info;

You can optimize this and only assign xdp_rxq_info once per NAPI.  E.g.
if you "allocate" struct xdp_buff on the callers stack, and pass it in
as a pointer.

> +
> +	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +	rcu_read_unlock();
> +
> +	offset = (u8 *)xdp.data - *ehp;
> +
> +	switch (xdp_act) {
> +	case XDP_PASS:
> +		/* Fix up rx prefix. */
> +		if (offset) {
> +			*ehp += offset;
> +			rx_buf->page_offset += offset;
> +			rx_buf->len -= offset;
> +			memcpy(*ehp - efx->rx_prefix_size, rx_prefix,
> +			       efx->rx_prefix_size);
> +		}
> +		break;
> +
> +	case XDP_TX:
> +		return -EOPNOTSUPP;
> +
> +	case XDP_REDIRECT:
> +		err = xdp_do_redirect(efx->net_dev, &xdp, xdp_prog);
> +		if (unlikely(err)) {
> +			efx_free_rx_buffers(rx_queue, rx_buf, 1);
> +			if (net_ratelimit())
> +				netif_err(efx, rx_err, efx->net_dev,
> +					  "XDP redirect failed (%d)\n", err);
> +		}
> +		break;
> +
> +	default:
> +		bpf_warn_invalid_xdp_action(xdp_act);
> +		/* Fall through */
> +	case XDP_ABORTED:

You are missing a tracepoint to catch ABORTED, e.g:
  trace_xdp_exception(netdev, xdp_prog, xdp_act);

> +		efx_free_rx_buffers(rx_queue, rx_buf, 1);
> +		break;

You can do a /* Fall through */ to case XDP_DROP.

> +	case XDP_DROP:
> +		efx_free_rx_buffers(rx_queue, rx_buf, 1);
> +		break;
> +	}
> +
> +	return xdp_act == XDP_PASS;
> +}

You can verify/test tracepoint for ABORTED as described here:

 https://github.com/xdp-project/xdp-tutorial/tree/master/basic02-prog-by-name#assignment-2-add-xdp_abort-program


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