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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200511185914.4oma2wbia4ukpfdr@kafai-mbp.dhcp.thefacebook.com>
Date:   Mon, 11 May 2020 11:59:14 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Jakub Sitnicki <jakub@...udflare.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <dccp@...r.kernel.org>, <kernel-team@...udflare.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Gerrit Renker <gerrit@....abdn.ac.uk>,
        Jakub Kicinski <kuba@...nel.org>,
        Marek Majkowski <marek@...udflare.com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: Re: [PATCH bpf-next 02/17] bpf: Introduce SK_LOOKUP program type
 with a dedicated attach point

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.

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?

Regardless, this details will be useful in the helper's doc.