netdev - Re: [PATCH net-next v3 6/9] tsnep: Support XDP BPF program setup

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 5 Jan 2023 18:24:14 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 6/9] tsnep: Support XDP BPF program setup

From: Gerhard Engleder <gerhard@...leder-embedded.com>
Date: Wed Jan 04 2023 20:41:29 GMT+0100

> Implement setup of BPF programs for XDP RX path with command
> XDP_SETUP_PROG of ndo_bpf(). This is prework for XDP RX path support.
> 
> tsnep_netdev_close() is called directly during BPF program setup. Add
> netif_carrier_off() and netif_tx_stop_all_queues() calls to signal to
> network stack that device is down. Otherwise network stack would
> continue transmitting pakets.
> 
> Return value of tsnep_netdev_open() is not checked during BPF program
> setup like in other drivers. Forwarding the return value would result in
> a bpf_prog_put() call in dev_xdp_install(), which would make removal of
> BPF program necessary.
> 
> If tsnep_netdev_open() fails during BPF program setup, then the network
> stack would call tsnep_netdev_close() anyway. Thus, tsnep_netdev_close()
> checks now if device is already down.
> 
> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/Makefile     |  2 +-
>  drivers/net/ethernet/engleder/tsnep.h      | 13 +++++++++++
>  drivers/net/ethernet/engleder/tsnep_main.c | 25 +++++++++++++++++---
>  drivers/net/ethernet/engleder/tsnep_xdp.c  | 27 ++++++++++++++++++++++
>  4 files changed, 63 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c
> 
> diff --git a/drivers/net/ethernet/engleder/Makefile b/drivers/net/ethernet/engleder/Makefile
> index b6e3b16623de..0901801cfcc9 100644
> --- a/drivers/net/ethernet/engleder/Makefile
> +++ b/drivers/net/ethernet/engleder/Makefile
> @@ -6,5 +6,5 @@
>  obj-$(CONFIG_TSNEP) += tsnep.o
>  
>  tsnep-objs := tsnep_main.o tsnep_ethtool.o tsnep_ptp.o tsnep_tc.o \
> -	      tsnep_rxnfc.o $(tsnep-y)
> +	      tsnep_rxnfc.o tsnep_xdp.o $(tsnep-y)

Not related directly to the subject, but could be fixed in that commit I
hope: you don't need to add $(tsnep-y) to $(tsnep-objs), it gets added
automatically.

>  tsnep-$(CONFIG_TSNEP_SELFTESTS) += tsnep_selftests.o
> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
> index 29b04127f529..0e7fc36a64e1 100644
> --- a/drivers/net/ethernet/engleder/tsnep.h
> +++ b/drivers/net/ethernet/engleder/tsnep.h

[...]

> @@ -215,6 +220,14 @@ int tsnep_rxnfc_add_rule(struct tsnep_adapter *adapter,
>  int tsnep_rxnfc_del_rule(struct tsnep_adapter *adapter,
>  			 struct ethtool_rxnfc *cmd);
>  
> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
> +			 struct netlink_ext_ack *extack);
> +
> +static inline bool tsnep_xdp_is_enabled(struct tsnep_adapter *adapter)
> +{
> +	return !!adapter->xdp_prog;

Any concurrent access protection? READ_ONCE(), RCU?

> +}
> +
>  #if IS_ENABLED(CONFIG_TSNEP_SELFTESTS)
>  int tsnep_ethtool_get_test_count(void);
>  void tsnep_ethtool_get_test_strings(u8 *data);

[...]

> +static int tsnep_netdev_bpf(struct net_device *dev, struct netdev_bpf *bpf)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(dev);
> +
> +	switch (bpf->command) {
> +	case XDP_SETUP_PROG:
> +		return tsnep_xdp_setup_prog(adapter, bpf->prog, bpf->extack);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

So, after this commit, I'm able to install an XDP prog to an interface,
but it won't do anything. I think the patch could be moved to the end of
the series, so that it won't end up with such?

> +
>  static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
>  				 struct xdp_frame **xdp, u32 flags)
>  {

[...]

> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
> +			 struct netlink_ext_ack *extack)
> +{
> +	struct net_device *dev = adapter->netdev;
> +	bool if_running = netif_running(dev);
> +	struct bpf_prog *old_prog;
> +
> +	if (if_running)
> +		tsnep_netdev_close(dev);

You don't need to close the interface if `!prog == !old_prog`. I
wouldn't introduce redundant down-ups here and leave no possibility for
prog hotswapping.

> +
> +	old_prog = xchg(&adapter->xdp_prog, prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	if (if_running)
> +		tsnep_netdev_open(dev);
> +
> +	return 0;
> +}

Thanks,
Olek