netdev - Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once

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: <20170510154630.316010f9@cakuba.netronome.com>
Date:   Wed, 10 May 2017 15:46:30 -0700
From:   Jakub Kicinski <kubakici@...pl>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     davem@...emloft.net, alexei.starovoitov@...il.com,
        john.fastabend@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at
 once

On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote:
> > I understand the counter argument that from user space perspective it
> > would make things slightly more complicated because there would be two
> > conditions in which driver hook is used:
> >   1) DRV_MODE set on dump;
> >   2) flags attribute not present (old kernel).
> >
> > I'm concerned about number 2).  We can't simply depend on SKB_MODE
> > not being set because we may add more *_MODE flags in the future.  So
> > doing:
> >
> > if (flags & SKB_MODE)
> > 	printf("skb-mode");
> > else
> > 	printf("drv-mode");
> >
> > is not correct.  The flags attribute must not be present at all (think
> > HW_MODE flag).  But going further there can also be non-MODE flags,
> > like, say.. NEVER_TX, and then there may be flags present in dump,
> > and if SKB_MODE isn't be set, the mode could be some new MODE user space
> > doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no
> > way to tell :S  
> 
> Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for
> dumping we're wasting bit space for flags we would never dump back
> such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future
> flags that are only relevant for loading, but never for dumping).
> Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE,
> we can still change this, since it's not too late.
> 
> How about the following proposal: IFLA_XDP_ATTACHED we have as-is
> (need to keep that anyway), if that is true, it means "something
> is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE
> (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1).
> I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS
> back, separating both attrs would avoid this hassle of what current
> or future load flag fits for dump as well and which not. Wdyt?

I really like the idea of not reusing IFLA_XDP_FLAGS for dumps!  New
enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100%
off-limits either?  4.11 would report (0) - driver supports XDP but
nothing is attached, (1) - something attached at the driver, could we
make (2) mean - something attached at generic XDP?  Would that be
considered breaking the ABI, are we bound to boolean semantics?