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

[Daniel Borkmann <daniel@iogearbox.net>]

On 05/11/2017 12:46 AM, Jakub Kicinski wrote:
> 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?

I like the idea, it would also render IFLA_XDP_ATTACHED not useless
or redundant then. Older binaries check !rta_getattr_u8(tb[IFLA_XDP_ATTACHED])
whether something is attached or not at XDP layer. So they won't know
IFLA_XDP_FLAGS attr either to make a more fine-grained distinction about
what mode. That seems actually cleaner to me, will give it a try.

Thanks,
Daniel