BPF Archive on lore.kernel.org
 help / color / Atom feed
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
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/

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-29  9:56 Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 2/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 3/6] bpf: sockmap: check value of unused args to BPF_PROG_ATTACH Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program Lorenz Bauer
2020-07-08  1:30   ` Martin KaFai Lau
2020-06-29  9:56 ` [PATCH bpf v2 5/6] selftests: bpf: pass program and target_fd in flow_dissector_reattach Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 6/6] selftests: bpf: pass program to bpf_prog_detach in flow_dissector Lorenz Bauer
2020-06-30  5:48 ` [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Yonghong Song
2020-06-30  8:39   ` Lorenz Bauer
2020-06-30 15:08     ` Yonghong Song
2020-06-30 15:50       ` Lorenz Bauer
2020-06-30 18:00 ` Alexei Starovoitov
2020-06-30 18:31   ` Jakub Sitnicki [this message]
2020-06-30 18:42     ` Alexei Starovoitov
2020-07-01  7:45       ` Jakub Sitnicki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878sg4mmlm.fsf@cloudflare.com \
    --to=jakub@cloudflare.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=john.fastabend@gmail.com \
    --cc=kernel-team@cloudflare.com \
    --cc=lmb@cloudflare.com \
    --cc=sdf@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git