netdev - Re: [RFC net-next 6/8] nfp: bpf: add support for XDP_FLAGS_HW

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] [day] [month] [year] [list]

Date:   Tue, 20 Jun 2017 02:09:54 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <kubakici@...pl>
CC:     netdev@...r.kernel.org, davem@...emloft.net, kafai@...com,
        brouer@...hat.com, oss-drivers@...ronome.com
Subject: Re: [RFC net-next 6/8] nfp: bpf: add support for XDP_FLAGS_HW_MODE

On 06/20/2017 02:01 AM, Jakub Kicinski wrote:
> On Tue, 20 Jun 2017 01:50:17 +0200, Daniel Borkmann wrote:
>> On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
>>> Respect the XDP_FLAGS_HW_MODE.  When it's set install the program
>>> on the NIC and skip enabling XDP in the driver.
>>>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>>> ---
>>>    drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++++++---
>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
>>> index 68648e312129..c5903b6e58c5 100644
>>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
>>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
>>> @@ -3310,19 +3310,22 @@ static int
>>>    nfp_net_xdp_setup(struct nfp_net *nn, struct bpf_prog *prog, u32 flags,
>>>    		  struct netlink_ext_ack *extack)
>>>    {
>>> -	struct bpf_prog *offload_prog;
>>> +	struct bpf_prog *drv_prog, *offload_prog;
>>>    	int err;
>>>
>>>    	if (nn->xdp_prog && (flags ^ nn->xdp_flags) & XDP_FLAGS_MODES)
>>>    		return -EBUSY;
>>>
>>> +	drv_prog     = flags & XDP_FLAGS_HW_MODE  ? NULL : prog;
>>>    	offload_prog = flags & XDP_FLAGS_DRV_MODE ? NULL : prog;
>>
>> Can you make this assumption here? If dev_change_xdp_fd() is called
>> without XDP_FLAGS_HW_MODE or XDP_FLAGS_DRV_MODE flags, then we set prog
>> to both, drv_prog and offload_prog. Is this expected?
>>
>> Maybe in nfp_net_xdp_setup() check for !hweight32(xdp_flags & XDP_FLAGS_MODES)
>> and then set flags |= XDP_FLAGS_DRV_MODE before both assignments?
>
> I thought we did want both.  In case the program is loaded to both the
> HW/FW will mark the packets with BPF bit in the descriptor so that they
> are not processed twice.  But the driver path will be configured for
> running bpf and when user replaces the program with one which cannot be
> offloaded the driver will not have to reconfigure itself.

Okay, that's a good point ... so that you can just use xchg() later on.
Probably worth explaining this rationale in a short comment.