Re: [PATCH bpf-next v2 2/5] af_unix: add unix_stream_proto for sockmap

From: Cong Wang <xiyou.wangcong@gmail.com>
To: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Jiang Wang <jiang.wang@bytedance.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	"Cong Wang ." <cong.wang@bytedance.com>,
	Xiongchun Duan <duanxiongchun@bytedance.com>,
	xieyongji@bytedance.com, chaiwen.cc@bytedance.com,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Lorenz Bauer <lmb@cloudflare.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, KP Singh <kpsingh@kernel.org>,
	Shuah Khan <shuah@kernel.org>,
	Johan Almbladh <johan.almbladh@anyfinetworks.com>,
	LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 2/5] af_unix: add unix_stream_proto for sockmap
Date: Sat, 31 Jul 2021 11:23:30 -0700	[thread overview]
Message-ID: <CAM_iQpVepKnEr_89XFiwH_8NBm12OUwT=H-AH8tbaESTpwaqMw@mail.gmail.com> (raw)
In-Reply-To: <875ywropno.fsf@cloudflare.com>

On Fri, Jul 30, 2021 at 7:14 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Thu, Jul 29, 2021 at 11:23 PM CEST, Jiang Wang wrote:
> > Previously, sockmap for AF_UNIX protocol only supports
> > dgram type. This patch add unix stream type support, which
> > is similar to unix_dgram_proto. To support sockmap, dgram
> > and stream cannot share the same unix_proto anymore, because
> > they have different implementations, such as unhash for stream
> > type (which will remove closed or disconnected sockets from the map),
> > so rename unix_proto to unix_dgram_proto and add a new
> > unix_stream_proto.
> >
> > Also implement stream related sockmap functions.
> > And add dgram key words to those dgram specific functions.
> >
> > Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> > Reviewed-by: Cong Wang <cong.wang@bytedance.com>
> > ---
>
> It seems that with commit c63829182c37 ("af_unix: Implement
> ->psock_update_sk_prot()") we have enabled inserting dgram, stream, and
> seqpacket UNIX sockets into sockmap.
>
> After all, in ->map_update_elem we only check if
> sk->sk_prot->psock_update_sk_prot is set (sock_map_sk_is_suitable).

Excellent point. I should check the sock type in unix_bpf_update_proto(),
and will send a fix.

>
> Socket can be in listening, established or disconnected (TCP_CLOSE)
> state, that is before bind+listen/connect, or after connect(AF_UNSPEC).
>
> For connection-oriented socket types (stream, seqpacket) there's not
> much you can do with disconnected sockets. I think we should limit the
> allowed states to listening and established for UNIX domain, as we do
> for TCP.

I think we should use ->unhash() to remove those connection-oriented
sockets, like TCP.

>
> AFAIU we also seem to be already allowing redirect to connected stream
> (and dgram, and seqpacket) UNIX sockets. sock_map_redirect_allowed()
> checks only if a socket is in TCP_ESTABLISHED state for anything else
> than TCP. Not sure what it leads to, though.

The goal is to keep all stream sockets like TCP, which only allows
established ones to stay in sockmap. For dgram, any socket state is
allowed to add to map but only established ones are allowed to redirect.

BTW, we do not have any intention to support Unix seqpacket socket
or any seqpacket.

Thanks.