netdev - Re: [PATCH net-next] net: mlx4: Add support for XDP

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: <8d7773a0-054a-84d5-e0b6-66a13509149e@gmail.com>
Date:   Sun, 26 Sep 2021 10:26:47 +0300
From:   Tariq Toukan <ttoukan.linux@...il.com>
To:     Joshua Roys <roysjosh@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: mlx4: Add support for XDP_REDIRECT

Hi,

Thanks for your patch.
It was submitted and accepted during the weekend, in our off-hours (we 
work Sunday - Thursday).
Please check my comments below.

On 9/23/2021 7:10 PM, Joshua Roys wrote:

Empty commit message? No feature description, motivation, performance 
numbers, etc... That's bad.

> Signed-off-by: Joshua Roys <roysjosh@...il.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_rx.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> This is a pattern-match commit, based off of the mlx4 XDP_TX and other
> drivers' XDP_REDIRECT enablement patches. The goal was to get AF_XDP
> working in VPP and this was successful. Tested with a CX3.
> 

Your comment here doesn't get into the git commit log. It can't replace 
the patch description above.

> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index 7f6d3b82c29b..557d7daac2d3 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -669,6 +669,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>   	struct bpf_prog *xdp_prog;
>   	int cq_ring = cq->ring;
>   	bool doorbell_pending;
> +	bool xdp_redir_flush;
>   	struct mlx4_cqe *cqe;
>   	struct xdp_buff xdp;
>   	int polled = 0;
> @@ -682,6 +683,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>   	xdp_prog = rcu_dereference_bh(ring->xdp_prog);
>   	xdp_init_buff(&xdp, priv->frag_info[0].frag_stride, &ring->xdp_rxq);
>   	doorbell_pending = false;
> +	xdp_redir_flush = false;
>   
>   	/* We assume a 1:1 mapping between CQEs and Rx descriptors, so Rx
>   	 * descriptor offset can be deduced from the CQE index instead of
> @@ -790,6 +792,14 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>   			switch (act) {
>   			case XDP_PASS:
>   				break;
> +			case XDP_REDIRECT:
> +				if (xdp_do_redirect(dev, &xdp, xdp_prog) >= 0) {

xdp_do_redirect returns a negative error code, or zero on success.
The >= 0 comparison looks strange and doesn't fit to the code style.
Simply use: "if (xdp_do_redirect(...))"


> +					xdp_redir_flush = true;
> +					frags[0].page = NULL;
> +					goto next;
> +				}
> +				trace_xdp_exception(dev, xdp_prog, act);
> +				goto xdp_drop_no_cnt;

You didn't add the required stats to count packets going through the new 
xdp_redirect flow. Every incoming packet *MUST* increase some counter. 
Please add it.

>   			case XDP_TX:
>   				if (likely(!mlx4_en_xmit_frame(ring, frags, priv,
>   							length, cq_ring,
> @@ -897,6 +907,9 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>   			break;
>   	}
>   
> +	if (xdp_redir_flush)
> +		xdp_do_flush();
> +
>   	if (likely(polled)) {
>   		if (doorbell_pending) {
>   			priv->tx_cq[TX_XDP][cq_ring]->xdp_busy = true;
> 

Thanks,
Tariq