netdev - Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,

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:   Mon, 03 Jun 2019 07:58:02 -0700
From:   "Jonathan Lemon" <jlemon@...gsvamp.com>
To:     "Björn Töpel" <bjorn.topel@...il.com>
Cc:     "Toke Høiland-Jørgensen" <toke@...hat.com>,
        "Alexei Starovoitov" <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>,
        "Björn Töpel" <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "Jesper Dangaard Brouer" <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>,
        "Jakub Kicinski" <jakub.kicinski@...ronome.com>,
        "Saeed Mahameed" <saeedm@...lanox.com>
Subject: Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW}
 to netdev

On 3 Jun 2019, at 1:39, Björn Töpel wrote:

> On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@...gsvamp.com> 
> wrote:
>>
>> On 31 May 2019, at 2:42, Björn Töpel wrote:
>>
>>> From: Björn Töpel <bjorn.topel@...el.com>
>>>
>>> All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
>>> command of ndo_bpf. The query code is fairly generic. This commit
>>> refactors the query code up from the drivers to the netdev level.
>>>
>>> The struct net_device has gained two new members: xdp_prog_hw and
>>> xdp_flags. The former is the offloaded XDP program, if any, and the
>>> latter tracks the flags that the supplied when attaching the XDP
>>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>>>
>>> The xdp_prog member, previously only used for SKB_MODE, is shared 
>>> with
>>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
>>> mutually exclusive. To differentiate between the two modes, a new
>>> internal flag is introduced as well.
>>
>> I'm not entirely clear why this new flag is needed - GENERIC seems to
>> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>>
>> If the user does not explicitly specify a type (skb|drv|hw), then the
>> command should choose the correct type and then behave as if this 
>> type
>> was specified.
>>
>
> Yes, this is kind of hairy.
>
> SKB and DRV are mutually exclusive, but HW is not. IOW, valid options 
> are:
> SKB, DRV, HW, SKB+HW DRV+HW.

Fair enough, that was the understanding that I had from the code, 
although I'm not sure about the usage of SKB+HW mode.



>
> What complicates things further, is that SKB and DRV can be implicitly
> (auto/no flags) or explicitly enabled (flags).
>
> If a user doesn't pass any flags, the "best supported mode" should be
> selected. If this "auto mode" is used, it should be seen as a third
> mode. E.g.
>
> ip link set dev eth0 xdp on -- OK
> ip link set dev eth0 xdp off -- OK
>
> ip link set dev eth0 xdp on -- OK # generic auto selected
> ip link set dev eth0 xdpgeneric off -- NOK, bad flags
>
> ip link set dev eth0 xdp on -- OK # drv auto selected
> ip link set dev eth0 xdpdrv off -- NOK, bad flags
>
> ...and so on. The idea is that a user should use the same set of flags 
> always.

I'm not sure about this.  The "xdp" mode shouldn't be treated as a 
separate mode, it should be "best supported mode", as indicated above.  
 From my view, it should select the appropriate mode, and then proceed 
as if the user had specified that mode, rather than being treated as an 
independent mode.

ip link set dev eth0 xdp on		- OK
ip link set dev eth0 xdp off	- OK

ip link set dev eth0 xdp on 	- OK, selected dev
ip link set dev eth0 xdpgeneric off - NOK, not running
ip link set dev eth0 xdpdrv off	- OK




> The internal "GENERIC" flag is only to determine if the xdp_prog
> represents a DRV version or SKB version. Maybe it would be clearer
> just to add an additional xdp_prog_drv to the net_device, instead?

I'd go the other way, and remove GENERIC, leaving only SKB, DRV, and HW.
The appropriate mode flag (SKB|DRV) is enough to indicate the type of 
xdp_prog.
-- 
Jonathan