Re: [PATCH bpf-next 02/17] bpf: Introduce SK_LOOKUP program type with a dedicated attach point

On Mon, May 11, 2020 at 10:54 PM CEST, Martin KaFai Lau wrote:
> On Mon, May 11, 2020 at 09:26:02PM +0200, Jakub Sitnicki wrote:
>> On Mon, May 11, 2020 at 08:59 PM CEST, Martin KaFai Lau wrote:
>> > On Mon, May 11, 2020 at 11:08:15AM +0200, Jakub Sitnicki wrote:
>> >> 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.
>> > Not suggesting the sk_assign helper has to support TCP_ESTABLISHED in TCP
>> > if there is no use case for it.
>>
>> Ack, I didn't think you were. Just explored the consequences.
>>
>> > Do you have a use case on supporting TCP_ESTABLISHED sk in UDP?
>> > From the cover letter use cases, it is not clear to me it is
>> > required.
>> >
>> > or both should only support unconnected sk?
>>
>> No, we don't have a use case for selecting a connected UDP socket.
>>
>> I left it as a possiblity because __udp[46]_lib_lookup, where BPF
>> sk_lookup is invoked from, can return one.
>>
>> Perhaps the user would like to connect the selected receiving socket
>> (for instance to itself) to ensure its not used for TX?
>>
>> I've pulled this scenario out of the hat. Happy to limit bpf_sk_assign
>> to select only unconnected UDP sockets, if returning a connected one
>> doesn't make sense.
> OTOH, my concern is:
> TCP's SK_LOOKUP can override the kernel choice on TCP_LISTEN sk.
> UDP's SK_LOOKUP can override the kernel choice on unconnected sk but
> not the connected sk.
>
> It could be quite confusing to bpf user if a bpf_prog was written to return
> both connected and unconnected UDP sk and logically expect both
> will be done before the kernel's choice.
>

That's a fair point. I've been looking at this from the PoV of in-kernel
callers of udp socket lookup, which now seems wrong.

I agree it would a be surprising if not confusing UAPI. Will limit it to
just unconnected UDP in v3.

Thanks for raising the concern,
Jakub