[PATCH v5 2/9] landlock: Control pathname UNIX domain socket resolution by path

[Sebastian Andrzej Siewior]

On 2026-03-08 10:18:04 [+0100], Mickaël Salaün wrote:
> > > dom_other, but please double check.  This lockless call should be made
> > > clear in the LSM hook.  It's OK to not lock the socket before
> > > security_unix_find() (1) because no LSM might implement and (2) they
> > > might not need to lock the socket (e.g. if the caller is not sandboxed).
> > > 
> > > The updated code should look something like this:
> > > 
> > > unix_state_unlock(other);
> 
> unix_state_lock(other) of course...
> 
> > > if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket)) {
> > > 	unix_state_unlock(other);
> > > 	return 0;
> > > }
> > > 
> > > dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
> > > unix_state_unlock(other);
> > 
> > Thank you for spotting the locking concern!
> > 
> > @anyone from netdev, could you please advise on the correct locking
> > approach here?

It is hard to tell where your "other" is from. So it is not clear to me
if the sock can be closed from the other side. If it can then sk_socket
becomes NULL and everything afterwards will be gone.

Therefore checking for SOCK_DEAD under unix_state_lock() looks sane.

> > * Do we need ot check SOCK_DEAD?
> > 
> >   You are saying that we need to do that, but it's not clear to me
> >   why.
> > 
> >   If you look at the places where unix_find_other() is called in
> >   af_unix.c, then you'll find that all of them check for SOCK_DEAD and
> >   then restart from unix_find_other() if they do actually discover
> >   that the socket is dead.  I think that this is catching this race
> >   condition scenario:
> > 
> >   * a server socket exists and is alive
> >   * A client connects: af_unix.c's unix_stream_connect() calls
> >     unix_find_other() and finds the server socket...
> >   * (Concurrently): The server closes the socket and enters
> >     unix_release_sock().  This function:
> >     1. disassociates the server sock from the named socket inode
> >        number in the hash table (=> future unix_find_other() calls
> >        will fail).
> >     2. calls sock_orphan(), which sets SOCK_DEAD.
> >   * ...(client connection resuming): unix_stream_connect() continues,
> >     grabs the unix_state_lock(), which apparently protects everything
> >     here, checks that the socket is not dead - and discovers that it
> >     IS suddenly dead.  This was not supposed to happen.  The code
> >     recovers from that by retrying everything starting with the
> >     unix_find_other() call.  From unix_release_sock(), we know that
> >     the inode is not associated with the sock any more -- so the
> >     unix_find_socket_by_inode() call should be failing on the next
> >     attempt.
> > 
> >   (This works with unix_dgram_connect() and unix_dgram_sendmsg() as
> >   well.)
> > 
> >   The comments next to the SOCK_DEAD checks are also suggesting this.

Sure. You are not the owner I guess. So you hold a reference on it but
the owner can still close it.

> > 
> > * What lock to use
> > 
> >   I am having trouble reasoning about what lock is used for what in
> >   this code.
> 
> It's not clear to me neither, and it looks like it's not consistent
> across protocols.
> 
> >   
> >   Is it possible that the lock protecting ->sk_socket is the
> >   ->sk_callback_lock, not the unix_state_lock()?  The only callers to
> >   sk_set_socket are either sock_orphan/sock_graft (both grabbing
> >   sk_callback_lock), or they are creating new struct sock objects that
> >   they own exclusively, and don't need locks yet.
> > 
> >   Admittedly, in af_unix.c, sock_orphan() and sock_graft() only get
> >   called in contexts where the unix_state_lock() is held, so it would
> >   probably work as well to lock that, but it is maybe a more
> >   fine-grained approach to use sk_callback_lock?

This is correct. Since only sock_orphan() is used you could go for
sk_callback_lock. For simplicity you could stick to the former lock
which will be accessed later any way. Either of the two block setting of
DEAD.

> > So... how about a scheme where we only check for ->sk_socket not being
> > NULL:
> > 
> > read_lock_bh(&other->sk_callback_lock);
> > struct sock *other_sk = other->sk_socket;
> > if (!other_sk) {
> > 	read_unlock_bh(&other->sk_callback_lock);
> > 	return 0;
> > }
> > /* XXX double check whether we need a lock here too */
> > struct file *file = other_sk->file;
> > if (!other_file) {
> > 	read_unlock_bh(&other->sk_callback_lock);
> > 	return 0;
> > }
> > read_unlock_bh(&other->sk_callback_lock);
> > 
> > If this fails, that would in my understanding also be because the
> > socket has died after the path lookup.  We'd then return 0 (success),
> > because we know that the surrounding SOCK_DEAD logic will repeat
> > everything starting from the path lookup operation (this time likely
> > failing with ECONNREFUSED, but maybe also with a success, if another
> > server process was quick enough).
> > 
> > Does this sound reasonable?

So if SOCK_DEAD is not set while the lock is held you can reference the
chain without second thoughts.

> Actually, since commit 983512f3a87f ("net: Drop the lock in
> skb_may_tx_timestamp()"), we can just use RCU + READ_ONCE(sk_socket) +
> READ_ONCE(file).  The socket and file should only be freed after the RCU
> grace periode.  As a safeguard, this commit should be a Depends-on.

This is what I concluded. The commit in question did not change the
situation. But if this spreads more I would suggest a helper so that all
user of this short cut can be easily identified. And yes, RCU would be a
key requirement.

> However, it is safer to return -ECONNREFULED when sk_socket or file are
> NULL.
> 
> I would be good to hear from netdev folks though.
> 
> TIL, there is an LSM hook for sock_graft().
> 
> > –Günther

Sebastian