All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Sitnicki <jakub@cloudflare.com>
To: Martin KaFai Lau <kafai@fb.com>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
	dccp@vger.kernel.org, kernel-team@cloudflare.com,
	Alexei Starovoitov <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Gerrit Renker <gerrit@erg.abdn.ac.uk>,
	Jakub Kicinski <kuba@kernel.org>,
	Marek Majkowski <marek@cloudflare.com>,
	Lorenz Bauer <lmb@cloudflare.com>
Subject: Re: [PATCH bpf-next 02/17] bpf: Introduce SK_LOOKUP program type with a dedicated attach point
Date: Mon, 11 May 2020 11:08:15 +0200	[thread overview]
Message-ID: <877dxivny8.fsf@cloudflare.com> (raw)
In-Reply-To: <20200508183928.ofudkphlb3vgpute@kafai-mbp.dhcp.thefacebook.com>

On Fri, May 08, 2020 at 08:39 PM CEST, Martin KaFai Lau wrote:
> On Fri, May 08, 2020 at 12:45:14PM +0200, Jakub Sitnicki wrote:
>> On Fri, May 08, 2020 at 09:06 AM CEST, Martin KaFai Lau wrote:
>> > On Wed, May 06, 2020 at 02:54:58PM +0200, Jakub Sitnicki wrote:

[...]

>> >> +		return -ESOCKTNOSUPPORT;
>> >> +
>> >> +	/* Check if socket is suitable for packet L3/L4 protocol */
>> >> +	if (sk->sk_protocol != ctx->protocol)
>> >> +		return -EPROTOTYPE;
>> >> +	if (sk->sk_family != ctx->family &&
>> >> +	    (sk->sk_family == AF_INET || ipv6_only_sock(sk)))
>> >> +		return -EAFNOSUPPORT;
>> >> +
>> >> +	/* Select socket as lookup result */
>> >> +	ctx->selected_sk = sk;
>> > Could sk be a TCP_ESTABLISHED sk?
>>
>> Yes, and what's worse, it could be ref-counted. This is a bug. I should
>> be rejecting ref counted sockets here.
> Agree. ref-counted (i.e. checking rcu protected or not) is the right check
> here.
>
> An unrelated quick thought, it may still be fine for the
> TCP_ESTABLISHED tcp_sk returned from sock_map because of the
> "call_rcu(&psock->rcu, sk_psock_destroy);" in sk_psock_drop().
> I was more thinking about in the future, what if this helper can take
> other sk not coming from sock_map.

I see, psock holds a sock reference and will not release it until a full
grace period has elapsed.

Even if holding a ref wasn't a problem, I'm not sure if returning a
TCP_ESTABLISHED socket wouldn't trip up callers of inet_lookup_listener
(tcp_v4_rcv and nf_tproxy_handle_time_wait4), that look for a listener
when processing a SYN to TIME_WAIT socket.

WARNING: multiple messages have this Message-ID (diff)
From: Jakub Sitnicki <jakub@cloudflare.com>
To: dccp@vger.kernel.org
Subject: Re: [PATCH bpf-next 02/17] bpf: Introduce SK_LOOKUP program type with a dedicated attach point
Date: Mon, 11 May 2020 09:08:15 +0000	[thread overview]
Message-ID: <877dxivny8.fsf@cloudflare.com> (raw)
In-Reply-To: <20200506125514.1020829-3-jakub@cloudflare.com>

On Fri, May 08, 2020 at 08:39 PM CEST, Martin KaFai Lau wrote:
> On Fri, May 08, 2020 at 12:45:14PM +0200, Jakub Sitnicki wrote:
>> On Fri, May 08, 2020 at 09:06 AM CEST, Martin KaFai Lau wrote:
>> > On Wed, May 06, 2020 at 02:54:58PM +0200, Jakub Sitnicki wrote:

[...]

>> >> +		return -ESOCKTNOSUPPORT;
>> >> +
>> >> +	/* Check if socket is suitable for packet L3/L4 protocol */
>> >> +	if (sk->sk_protocol != ctx->protocol)
>> >> +		return -EPROTOTYPE;
>> >> +	if (sk->sk_family != ctx->family &&
>> >> +	    (sk->sk_family = AF_INET || ipv6_only_sock(sk)))
>> >> +		return -EAFNOSUPPORT;
>> >> +
>> >> +	/* Select socket as lookup result */
>> >> +	ctx->selected_sk = sk;
>> > Could sk be a TCP_ESTABLISHED sk?
>>
>> Yes, and what's worse, it could be ref-counted. This is a bug. I should
>> be rejecting ref counted sockets here.
> Agree. ref-counted (i.e. checking rcu protected or not) is the right check
> here.
>
> An unrelated quick thought, it may still be fine for the
> TCP_ESTABLISHED tcp_sk returned from sock_map because of the
> "call_rcu(&psock->rcu, sk_psock_destroy);" in sk_psock_drop().
> I was more thinking about in the future, what if this helper can take
> other sk not coming from sock_map.

I see, psock holds a sock reference and will not release it until a full
grace period has elapsed.

Even if holding a ref wasn't a problem, I'm not sure if returning a
TCP_ESTABLISHED socket wouldn't trip up callers of inet_lookup_listener
(tcp_v4_rcv and nf_tproxy_handle_time_wait4), that look for a listener
when processing a SYN to TIME_WAIT socket.

  reply	other threads:[~2020-05-11  9:08 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-06 12:54 [PATCH bpf-next 00/17] Run a BPF program on socket lookup Jakub Sitnicki
2020-05-06 12:54 ` Jakub Sitnicki
2020-05-06 12:54 ` [PATCH bpf-next 01/17] flow_dissector: Extract attach/detach/query helpers Jakub Sitnicki
2020-05-06 12:54   ` Jakub Sitnicki
2020-05-06 12:54 ` [PATCH bpf-next 02/17] bpf: Introduce SK_LOOKUP program type with a dedicated attach point Jakub Sitnicki
2020-05-06 12:54   ` Jakub Sitnicki
2020-05-06 13:16   ` Lorenz Bauer
2020-05-06 13:16     ` Lorenz Bauer
2020-05-06 13:53     ` Jakub Sitnicki
2020-05-06 13:53       ` Jakub Sitnicki
2020-05-07 20:55       ` Martin KaFai Lau
2020-05-07 20:55         ` Martin KaFai Lau
2020-05-08  8:54         ` Jakub Sitnicki
2020-05-08  8:54           ` Jakub Sitnicki
2020-05-08  7:06   ` Martin KaFai Lau
2020-05-08  7:06     ` Martin KaFai Lau
2020-05-08 10:45     ` Jakub Sitnicki
2020-05-08 10:45       ` Jakub Sitnicki
2020-05-08 18:39       ` Martin KaFai Lau
2020-05-08 18:39         ` Martin KaFai Lau
2020-05-11  9:08         ` Jakub Sitnicki [this message]
2020-05-11  9:08           ` Jakub Sitnicki
2020-05-11 18:59           ` Martin KaFai Lau
2020-05-11 18:59             ` Martin KaFai Lau
2020-05-11 19:26             ` Jakub Sitnicki
2020-05-11 19:26               ` Jakub Sitnicki
2020-05-11 20:54               ` Martin KaFai Lau
2020-05-11 20:54                 ` Martin KaFai Lau
2020-05-12 14:16                 ` Jakub Sitnicki
2020-05-12 14:16                   ` Jakub Sitnicki
2020-05-06 12:54 ` [PATCH bpf-next 03/17] inet: Store layer 4 protocol in inet_hashinfo Jakub Sitnicki
2020-05-06 12:54   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 04/17] inet: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 05/17] inet: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 06/17] inet6: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 07/17] inet6: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 08/17] udp: Store layer 4 protocol in udp_table Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 09/17] udp: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 10/17] udp: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 11/17] udp6: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 12/17] udp6: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 13/17] bpf: Sync linux/bpf.h to tools/ Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 14/17] libbpf: Add support for SK_LOOKUP program type Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-08 17:41   ` Andrii Nakryiko
2020-05-08 17:41     ` Andrii Nakryiko
2020-05-08 17:52     ` Yonghong Song
2020-05-08 17:52       ` Yonghong Song
2020-05-08 17:59       ` Andrii Nakryiko
2020-05-08 17:59         ` Andrii Nakryiko
2020-05-11  8:12     ` Jakub Sitnicki
2020-05-11  8:12       ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 15/17] selftests/bpf: Add verifier tests for bpf_sk_lookup context access Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 16/17] selftests/bpf: Rename test_sk_lookup_kern.c to test_ref_track_kern.c Jakub Sitnicki
2020-05-06 12:55   ` Jakub Sitnicki
2020-05-06 12:55 ` [PATCH bpf-next 17/17] selftests/bpf: Tests for BPF_SK_LOOKUP attach point Jakub Sitnicki
2020-05-06 12:55   ` 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=877dxivny8.fsf@cloudflare.com \
    --to=jakub@cloudflare.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dccp@vger.kernel.org \
    --cc=edumazet@google.com \
    --cc=gerrit@erg.abdn.ac.uk \
    --cc=kafai@fb.com \
    --cc=kernel-team@cloudflare.com \
    --cc=kuba@kernel.org \
    --cc=lmb@cloudflare.com \
    --cc=marek@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.