Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

From: Jakub Sitnicki <jakub@cloudflare.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Lorenz Bauer <lmb@cloudflare.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Stanislav Fomichev <sdf@google.com>,
	John Fastabend <john.fastabend@gmail.com>,
	kernel-team <kernel-team@cloudflare.com>,
	bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
Date: Tue, 30 Jun 2020 20:31:49 +0200	[thread overview]
Message-ID: <878sg4mmlm.fsf@cloudflare.com> (raw)
In-Reply-To: <CAADnVQ+VN6nUCQC51nByeKa+uHG=O-GyzeEpWQgJ8OP815RB2A@mail.gmail.com>

On Tue, Jun 30, 2020 at 08:00 PM CEST, Alexei Starovoitov wrote:
> On Mon, Jun 29, 2020 at 2:59 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>>
>> Both sockmap and flow_dissector ingnore various arguments passed to
>> BPF_PROG_ATTACH and BPF_PROG_DETACH. We can fix the attach case by
>> checking that the unused arguments are zero. I considered requiring
>> target_fd to be -1 instead of 0, but this leads to a lot of churn
>> in selftests. There is also precedent in that bpf_iter already
>> expects 0 for a similar field. I think that we can come up with a
>> work around for fd 0 should we need to in the future.
>>
>> The detach case is more problematic: both cgroups and lirc2 verify
>> that attach_bpf_fd matches the currently attached program. This
>> way you need access to the program fd to be able to remove it.
>> Neither sockmap nor flow_dissector do this. flow_dissector even
>> has a check for CAP_NET_ADMIN because of this. The patch set
>> addresses this by implementing the desired behaviour.
>>
>> There is a possibility for user space breakage: any callers that
>> don't provide the correct fd will fail with ENOENT. For sockmap
>> the risk is low: even the selftests assume that sockmap works
>> the way I described. For flow_dissector the story is less
>> straightforward, and the selftests use a variety of arguments.
>>
>> I've includes fixes tags for the oldest commits that allow an easy
>> backport, however the behaviour dates back to when sockmap and
>> flow_dissector were introduced. What is the best way to handle these?
>>
>> This set is based on top of Jakub's work "bpf, netns: Prepare
>> for multi-prog attachment" available at
>> https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/
>
> Folks, you should have used 'bpf' in the subject of Jakub's patches.
> I've dropped Jakub's set from bpf-next and re-applied to bpf tree.
> And applied this set on top.
> Thanks!

The preparatory work for multi-prog for netns programs [0] that landed
recently in bpf-next wasn't fixing anything, so I thought it was -next
material.

I've messed up because I've asked Lorenz to base his patchset on top of
mine, but didn't consider the fact that Lorenz's fixes are targeted for
v5.8 and earlier :-/

So alternatively, we could respin Lorenz patchset on top of 'bpf', if
you want, to untangle this situation.

But if you decide to keep my patchset in 'bpf', then can you please also
apply the today's fixup [1]? Or I can resend with correct subject prefix
tomorrow.

Thanks,
-jkbs

[0] https://lore.kernel.org/bpf/20200625141357.910330-1-jakub@cloudflare.com/
[1] https://lore.kernel.org/bpf/20200630164541.1329993-1-jakub@cloudflare.com/