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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b7b122e1-3be9-bee1-ee3d-1b4daa8893ac@engleder-embedded.com>
Date:   Thu, 5 Jan 2023 23:09:08 +0100
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.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

On 05.01.23 18:24, Alexander Lobakin wrote:
> 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.

I will fix that with this commit.

>>   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?

I assume this is about prog hotswapping which you mentioned below. Is
concurrent access protection needed without prog hotswapping?

>> +}
>> +
>>   #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?

My thinking was to first implement the base and the actual functionality
last. I can move it to the end.

>> +
>>   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.

If prog hotswapping is possible, then how shall it be ensured that the
old prog is not running anymore before 'bpf_prog_put(old_prog)' is
called? I don't understand how 'READ_ONCE(adapter->xdp_prog)' during RX
path and 'old_prog = xchg(&adapter->xdp_prog, prog)' during prog setup
can ensure that (e.g. in ixgbe driver).

Gerhard