Re: [PATCH 1/2] fs: Add bpf_sock_read_xattr() kfunc to read socket xattrs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing READ_ONCE() on lockless access to sock->file can lead to a TOCTOU NULL pointer dereference.
--

commit 9bf8c99c6cda345fa08537f5df5f36bc0aa367b9
Author: Christian Brauner <brauner@kernel.org>

fs: Add bpf_sock_read_xattr() kfunc to read socket xattrs

This commit introduces the bpf_sock_read_xattr() kfunc to allow BPF LSM
programs to locklessly read user.* extended attributes from a socket's
sockfs inode. The kfunc is registered as KF_RCU, and retrieves the
attributes using an RCU-protected rhashtable lookup.

Link: https://github.com/systemd/systemd/pull/40559 [1]

> diff --git a/net/socket.c b/net/socket.c
> index 9e8dc769ff7af..3566f8c8ea3fb 100644
> --- a/net/socket.c
> +++ b/net/socket.c
[ ... ]
> @@ -465,6 +465,31 @@ static const struct xattr_handler sockfs_user_xattr_handler = {
>  	.set = sockfs_user_xattr_set,
>  };
>  
> +/**
> + * sock_read_xattr - read a user.* xattr from a socket's sockfs inode
[ ... ]
> +int sock_read_xattr(struct socket *sock, const char *name, void *value, size_t size)
> +{
> +	struct file *file = sock->file;
> +	struct sockfs_inode *si;
> +
> +	if (!file || sock_from_file(file) != sock)

[Severity: High]
Can a concurrent close cause a NULL pointer dereference here?

Since bpf_sock_read_xattr() is registered with KF_RCU, BPF programs can
call it under RCU protection where the socket might not have a held file
reference. If the last file descriptor is closed concurrently,
__sock_release() can set sock->file = NULL without waiting for an RCU grace
period.

Because sock->file is read without READ_ONCE(), the compiler is permitted
to reload the pointer from memory. If the initial read for the !file check
sees a valid pointer, but a subsequent read for the inlined
sock_from_file(file) call sees NULL due to the concurrent modification,
sock_from_file() will attempt to dereference file->f_op on a NULL pointer.

Does this need struct file *file = READ_ONCE(sock->file) to prevent a
TOCTOU issue?

> +		return -EOPNOTSUPP;
> +
> +	si = SOCKFS_I(SOCK_INODE(sock));
> +	return simple_xattr_get(&sockfs_xa_cache, &si->xattrs, name, value, size);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-work-bpf-sock-xattr-v1-0-a1276f7c9da3@kernel.org?part=1