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

On Fri, Jul 30, 2021 at 7:14 AM Jakub Sitnicki [off-list ref] wrote:

On Thu, Jul 29, 2021 at 11:23 PM CEST, Jiang Wang wrote:

quoted

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 <redacted>
Reviewed-by: Cong Wang <redacted>
---

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.