netdev - Re: [PATCH net-next v3 5/7] net: lan966x: Update dma_dir of page_pool

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]

Date:   Tue, 22 Nov 2022 12:43:39 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, ast@...nel.org,
        daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next v3 5/7] net: lan966x: Update dma_dir of page_pool_params

From: Horatiu Vultur <horatiu.vultur@...rochip.com>
Date: Mon, 21 Nov 2022 22:28:48 +0100

> To add support for XDP_TX it is required to be able to write to the DMA
> area therefore it is required that the pages will be mapped using
> DMA_BIDIRECTIONAL flag.
> Therefore check if there are any xdp programs on the interfaces and in
> that case set DMA_BIDRECTIONAL otherwise use DMA_FROM_DEVICE.
> Therefore when a new XDP program is added it is required to redo the
> page_pool.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
> ---
>  .../ethernet/microchip/lan966x/lan966x_fdma.c | 29 ++++++++++++++----
>  .../ethernet/microchip/lan966x/lan966x_main.h |  2 ++
>  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 30 +++++++++++++++++++
>  3 files changed, 55 insertions(+), 6 deletions(-)

[...]

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> index 8ebde1eb6a09c..05c5a28206558 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> @@ -11,6 +11,8 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
>  	struct lan966x_port *port = netdev_priv(dev);
>  	struct lan966x *lan966x = port->lan966x;
>  	struct bpf_prog *old_prog;
> +	bool old_xdp, new_xdp;
> +	int err;
>  
>  	if (!lan966x->fdma) {
>  		NL_SET_ERR_MSG_MOD(xdp->extack,
> @@ -18,7 +20,20 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
>  		return -EOPNOTSUPP;
>  	}
>  
> +	old_xdp = lan966x_xdp_present(lan966x);
>  	old_prog = xchg(&port->xdp_prog, xdp->prog);
> +	new_xdp = lan966x_xdp_present(lan966x);
> +
> +	if (old_xdp != new_xdp)
> +		goto out;

Shouldn't it be the other way around? E.g. when there's no prog and
you're installing it or there is a prog and we're removing it from
the interface, DMA dir must be changed, so we reload the Pools, but
if `old_xdp == new_xdp` we should just hotswap them and goto out?

> +
> +	err = lan966x_fdma_reload_page_pool(lan966x);
> +	if (err) {
> +		xchg(&port->xdp_prog, old_prog);
> +		return err;
> +	}
> +
> +out:
>  	if (old_prog)
>  		bpf_prog_put(old_prog);
>  

[...]

> -- 
> 2.38.0

Thanks,
Olek