On 03/08/2019 12:06 PM, Björn Töpel wrote: > On Fri, 8 Mar 2019 at 11:46, Maciej Fijalkowski > <maciejromanfijalkowski@xxxxxxxxx> wrote: [...] >>>> So maybe check here also that only one particular flag is set by doing: >>>> >>>> if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1) >>>> return -EINVAL; >>>> >>>> just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c? >>> >>> We have flag semantic checks further down, and my rational was to >>> *only* check unknown flags first. IMO the current patch is easier to >>> understand, than your suggested one. >> >> Hmm thought that bailing out earlier would be better and we could drop the >> actual copy flags checks for shared umem. For xdp_umem_assign_dev() instead of >> passing flags you could just pass a boolean whether you're doing zero copy or >> not. And that brings up the question whether we really need a XDP_COPY flag? > > I'd prefer doing that as a follow-up patch. Ok, follow-up for Maciej's suggestion is fine, imho, it would definitely make it more obvious resp. future proof such that rejecting some invalid combination doesn't get missed under some branch. Given it's currently not the case aside from the fix, rolling this into your next patches for bpf-next might be best. In any case, fixes look good, so applied, thanks! > XDP_COPY is needed to explicitly enable copy-mode. No flags is "select > the best option", and COPY/ZEROCOPY is to explicitly select a mode.
