bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Sitnicki <jakub@cloudflare.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	bpf@vger.kernel.org, netdev@vger.kernel.org,
	kernel-team@cloudflare.com, Martin KaFai Lau <kafai@fb.com>
Subject: Re: [PATCH bpf-next 5/8] bpf: Allow selecting reuseport socket from a SOCKMAP
Date: Mon, 25 Nov 2019 11:40:41 +0100	[thread overview]
Message-ID: <87o8x0nsra.fsf@cloudflare.com> (raw)
In-Reply-To: <5ddb55c87d06c_79e12b0ab99325bc69@john-XPS-13-9370.notmuch>

On Mon, Nov 25, 2019 at 05:17 AM CET, John Fastabend wrote:
> Alexei Starovoitov wrote:
>> On Sat, Nov 23, 2019 at 12:07:48PM +0100, Jakub Sitnicki wrote:
>> > SOCKMAP now supports storing references to listening sockets. Nothing keeps
>> > us from using it as an array of sockets to select from in SK_REUSEPORT
>> > programs.
>> >
>> > Whitelist the map type with the BPF helper for selecting socket. However,
>> > impose a restriction that the selected socket needs to be a listening TCP
>> > socket or a bound UDP socket (connected or not).
>> >
>> > The only other map type that works with the BPF reuseport helper,
>> > REUSEPORT_SOCKARRAY, has a corresponding check in its update operation
>> > handler.
>> >
>> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> > ---
>
> [...]
>
>> > diff --git a/net/core/filter.c b/net/core/filter.c
>> > index 49ded4a7588a..e3fb77353248 100644
>> > --- a/net/core/filter.c
>> > +++ b/net/core/filter.c
>> > @@ -8723,6 +8723,8 @@ BPF_CALL_4(sk_select_reuseport, struct sk_reuseport_kern *, reuse_kern,
>> >  	selected_sk = map->ops->map_lookup_elem(map, key);
>> >  	if (!selected_sk)
>> >  		return -ENOENT;
>> > +	if (!sock_flag(selected_sk, SOCK_RCU_FREE))
>> > +		return -EINVAL;
>>
>> hmm. I wonder whether this breaks existing users...
>
> There is already this check in reuseport_array_update_check()
>
> 	/*
> 	 * sk must be hashed (i.e. listening in the TCP case or binded
> 	 * in the UDP case) and
> 	 * it must also be a SO_REUSEPORT sk (i.e. reuse cannot be NULL).
> 	 *
> 	 * Also, sk will be used in bpf helper that is protected by
> 	 * rcu_read_lock().
> 	 */
> 	if (!sock_flag(nsk, SOCK_RCU_FREE) || !sk_hashed(nsk) || !nsk_reuse)
> 		return -EINVAL;
>
> So I believe it should not cause any problems with existing users. Perhaps
> we could consolidate the checks a bit or move it into the update paths if we
> wanted. I assume Jakub was just ensuring we don't get here with SOCK_RCU_FREE
> set from any of the new paths now. I'll let him answer though.

That was exactly my thinking here.

REUSEPORT_SOCKARRAY can't be populated with sockets that don't have
SOCK_RCU_FREE set. This makes the flag check in sk_select_reuseport BPF
helper redundant for this map type.

SOCKMAP, OTOH, allows storing established TCP sockets, which don't have
SOCK_RCU_FREE flag and shouldn't be used as reuseport targets. The newly
added check protects us against it.

I have a couple tests in the last patch for it -
test_sockmap_reuseport_select_{listening,connected}. Admittedly, UDP is
not covered.

Not sure how we could go about moving the checks to the update path for
SOCKMAP. At update time we don't know if the map will be used with a
reuseport or a sk_{skb,msg} program.

-Jakub

>
>> Martin,
>> what do you think?
>
> More eyes the better.
>
>> Could you also take a look at other patches too?
>> In particular patch 7?
>>
>
> Agreed would be good to give 7/8 a look I'm not too familiar with the
> selftests there.

  reply	other threads:[~2019-11-25 10:40 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-23 11:07 [PATCH bpf-next 0/8] Extend SOCKMAP to store listening sockets Jakub Sitnicki
2019-11-23 11:07 ` [PATCH bpf-next 1/8] bpf, sockmap: Return socket cookie on lookup from syscall Jakub Sitnicki
2019-11-24  5:32   ` John Fastabend
2019-11-23 11:07 ` [PATCH bpf-next 2/8] bpf, sockmap: Let all kernel-land lookup values in SOCKMAP Jakub Sitnicki
2019-11-24  5:35   ` John Fastabend
2019-11-23 11:07 ` [PATCH bpf-next 3/8] bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP Jakub Sitnicki
2019-11-24  5:38   ` John Fastabend
2019-11-23 11:07 ` [PATCH bpf-next 4/8] bpf, sockmap: Don't let child socket inherit psock or its ops on copy Jakub Sitnicki
2019-11-24  5:56   ` John Fastabend
2019-11-25 22:38   ` Martin Lau
2019-11-26 15:54     ` Jakub Sitnicki
2019-11-26 17:16       ` Martin Lau
2019-11-26 18:36         ` Jakub Sitnicki
     [not found]           ` <87sglsfdda.fsf@cloudflare.com>
2019-12-11 17:20             ` Martin Lau
2019-12-12 11:27               ` Jakub Sitnicki
2019-12-12 19:23                 ` Martin Lau
2019-12-17 15:06                   ` Jakub Sitnicki
2019-11-26 18:43         ` John Fastabend
2019-11-27 22:18           ` Jakub Sitnicki
2019-11-23 11:07 ` [PATCH bpf-next 5/8] bpf: Allow selecting reuseport socket from a SOCKMAP Jakub Sitnicki
2019-11-24  5:57   ` John Fastabend
2019-11-25  1:24   ` Alexei Starovoitov
2019-11-25  4:17     ` John Fastabend
2019-11-25 10:40       ` Jakub Sitnicki [this message]
2019-11-25 22:07         ` Martin Lau
2019-11-26 14:30           ` Jakub Sitnicki
2019-11-26 19:03             ` Martin Lau
2019-11-27 21:34               ` Jakub Sitnicki
2019-11-23 11:07 ` [PATCH bpf-next 6/8] libbpf: Recognize SK_REUSEPORT programs from section name Jakub Sitnicki
2019-11-24  5:57   ` John Fastabend
2019-11-23 11:07 ` [PATCH bpf-next 7/8] selftests/bpf: Extend SK_REUSEPORT tests to cover SOCKMAP Jakub Sitnicki
2019-11-24  6:00   ` John Fastabend
2019-11-25 22:30   ` Martin Lau
2019-11-26 14:32     ` Jakub Sitnicki
2019-12-12 10:30     ` Jakub Sitnicki
2019-11-23 11:07 ` [PATCH bpf-next 8/8] selftests/bpf: Tests for SOCKMAP holding listening sockets Jakub Sitnicki
2019-11-24  6:04   ` John Fastabend
2019-11-24  6:10 ` [PATCH bpf-next 0/8] Extend SOCKMAP to store " John Fastabend
2019-11-25  9:22   ` 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=87o8x0nsra.fsf@cloudflare.com \
    --to=jakub@cloudflare.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kernel-team@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH bpf-next 5/8] bpf: Allow selecting reuseport socket from a SOCKMAP' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).