netdev - Re: [PATCH net-next v4 10/10] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <336b9f28bca980813310dd3007c862e9f738279e.camel@gmail.com>
Date:   Tue, 10 Jan 2023 09:33:59 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
        pabeni@...hat.com
Subject: Re: [PATCH net-next v4 10/10] tsnep: Support XDP BPF program setup

On Mon, 2023-01-09 at 20:15 +0100, Gerhard Engleder wrote:
> Implement setup of BPF programs for XDP RX path with command
> XDP_SETUP_PROG of ndo_bpf(). This is the final step 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.
> 
> Additionally remove $(tsnep-y) from $(tsnep-objs) because it is added
> automatically.
> 
> Test results with A53 1.2GHz:
> 
> XDP_DROP (samples/bpf/xdp1)
> proto 17:     883878 pkt/s
> 
> XDP_TX (samples/bpf/xdp2)
> proto 17:     255693 pkt/s
> 
> XDP_REDIRECT (samples/bpf/xdpsock)
>  sock0@...2:0 rxdrop xdp-drv
>                    pps            pkts           1.00
> rx                 855,582        5,404,523
> tx                 0              0
> 
> XDP_REDIRECT (samples/bpf/xdp_redirect)
> eth2->eth1         613,267 rx/s   0 err,drop/s   613,272 xmit/s
> 
> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/Makefile     |  2 +-
>  drivers/net/ethernet/engleder/tsnep.h      |  6 +++++
>  drivers/net/ethernet/engleder/tsnep_main.c | 25 ++++++++++++++++---
>  drivers/net/ethernet/engleder/tsnep_xdp.c  | 29 ++++++++++++++++++++++
>  4 files changed, 58 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c
> 
> 

<...>

> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -1373,7 +1373,7 @@ static void tsnep_free_irq(struct tsnep_queue *queue, bool first)
>  	memset(queue->name, 0, sizeof(queue->name));
>  }
>  
> -static int tsnep_netdev_open(struct net_device *netdev)
> +int tsnep_netdev_open(struct net_device *netdev)
>  {
>  	struct tsnep_adapter *adapter = netdev_priv(netdev);
>  	int tx_queue_index = 0;
> @@ -1436,6 +1436,8 @@ static int tsnep_netdev_open(struct net_device *netdev)
>  		tsnep_enable_irq(adapter, adapter->queue[i].irq_mask);
>  	}
>  
> +	netif_tx_start_all_queues(adapter->netdev);
> +
>  	clear_bit(__TSNEP_DOWN, &adapter->state);
>  
>  	return 0;
> @@ -1457,12 +1459,16 @@ static int tsnep_netdev_open(struct net_device *netdev)
>  	return retval;
>  }
>  
> -static int tsnep_netdev_close(struct net_device *netdev)
> +int tsnep_netdev_close(struct net_device *netdev)
>  {
>  	struct tsnep_adapter *adapter = netdev_priv(netdev);
>  	int i;
>  
> -	set_bit(__TSNEP_DOWN, &adapter->state);
> +	if (test_and_set_bit(__TSNEP_DOWN, &adapter->state))
> +		return 0;
> +
> +	netif_carrier_off(netdev);
> +	netif_tx_stop_all_queues(netdev);
>  

As I called out earlier the __TSNEP_DOWN is just !IFF_UP so you don't
need that bit.

The fact that netif_carrier_off is here also points out the fact that
the code in the Tx path isn't needed regarding __TSNEP_DOWN and you can
probably just check netif_carrier_ok if you need the check.

>  	tsnep_disable_irq(adapter, ECM_INT_LINK);
>  	tsnep_phy_close(adapter);
> @@ -1627,6 +1633,18 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
>  	return ns_to_ktime(timestamp);
>  }
>  
> +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;
> +	}
> +}
> +
>  static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
>  				 struct xdp_frame **xdp, u32 flags)
>  {
> @@ -1677,6 +1695,7 @@ static const struct net_device_ops tsnep_netdev_ops = {
>  	.ndo_set_features = tsnep_netdev_set_features,
>  	.ndo_get_tstamp = tsnep_netdev_get_tstamp,
>  	.ndo_setup_tc = tsnep_tc_setup,
> +	.ndo_bpf = tsnep_netdev_bpf,
>  	.ndo_xdp_xmit = tsnep_netdev_xdp_xmit,
>  };
>  
>