Re: [RFC PATCH v2 04/41] net: sock: read sk_uid through a seqcount

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: sock: write_seqcount_begin() called in preemptible sock_init_data_uid()
- [High] net: sock: raw seqcount_t under sk_callback_lock livelocks on PREEMPT_RT
--

commit e15d18b6d3adc57553d8944b44a103206aaa6a91
Author: Jan Koester <jan.koester@tuxist.de>

net: sock: read sk_uid through a seqcount

This commit introduces a sequence counter (sk_uid_seq) in struct sock to
protect lockless reads of sk_uid against concurrent updates in sock_graft()
and sockfs_setattr(). Writers update sk_uid via __sk_set_uid(), while readers
use sk_uid() to retry until a consistent value is read.

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 60ea55dc18854..0986d946659a8 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h

[ ... ]

> @@ -509,6 +511,7 @@ struct sock {
>  	u32			sk_priority;
>  	u32			sk_mark;
>  	kuid_t			sk_uid;
> +	seqcount_t		sk_uid_seq;
>  	u16			sk_protocol;
>  	u16			sk_type;
>  	struct dst_entry __rcu	*sk_dst_cache;
> @@ -2109,11 +2112,23 @@ static inline int sk_rx_queue_get(const struct sock *sk)
>  	return -1;
>  }
>  
> +/*
> + * sk_uid is too big for WRITE_ONCE(): writers hold sk_callback_lock (or own
> + * the socket alone, before it is published) and bump sk_uid_seq around the
> + * store, so sk_uid() can read it without a lock.
> + */
> +static inline void __sk_set_uid(struct sock *sk, kuid_t uid)
> +{
> +	write_seqcount_begin(&sk->sk_uid_seq);
> +	sk->sk_uid = uid;
> +	write_seqcount_end(&sk->sk_uid_seq);
> +}
> +
>  static inline void sk_set_socket(struct sock *sk, struct socket *sock)
>  {
>  	WRITE_ONCE(sk->sk_socket, sock);
>  	if (sock) {
> -		WRITE_ONCE(sk->sk_uid, SOCK_INODE(sock)->i_uid);
> +		__sk_set_uid(sk, SOCK_INODE(sock)->i_uid);
>  		WRITE_ONCE(sk->sk_ino, SOCK_INODE(sock)->i_ino);
>  	} else {
>  		/* Note: sk_uid is unchanged. */
> @@ -2161,8 +2176,15 @@ static inline u64 sock_i_ino(const struct sock *sk)
>  
>  static inline kuid_t sk_uid(const struct sock *sk)
>  {
> -	/* Paired with WRITE_ONCE() in sockfs_setattr() */
> -	return READ_ONCE(sk->sk_uid);
> +	unsigned int seq;
> +	kuid_t uid;
> +
> +	/* Paired with __sk_set_uid() */
> +	do {
> +		seq = read_seqcount_begin(&sk->sk_uid_seq);
> +		uid = sk->sk_uid;
> +	} while (read_seqcount_retry(&sk->sk_uid_seq, seq));
> +	return uid;
>  }

[Severity: High]
Could using a raw seqcount_t for sk_uid_seq cause preemption warnings and a
reader livelock on preempt-rt kernels?

Runtime writers such as sockfs_setattr(), sock_graft(), and
mptcp_sock_graft() serialize updates to sk->sk_uid with
write_lock_bh(&sk->sk_callback_lock) before calling __sk_set_uid() (directly
or via sk_set_socket()). On preempt-rt kernels, write_lock_bh() on an
rwlock_t does not disable preemption:

include/linux/rwlock_rt.h:write_lock_bh() {
	local_bh_disable();
	rt_write_lock(rwlock);
}

Because sk_uid_seq is a raw seqcount_t rather than a seqcount_rwlock_t
associated with sk_callback_lock, write_seqcount_begin(&sk->sk_uid_seq) does
not disable preemption and triggers lockdep_assert_preemption_disabled() in
__seqprop_assert():

include/linux/seqlock.h:__seqprop_assert() {
	lockdep_assert_preemption_disabled();
}

In addition, if a higher-priority task preempts the writer between
write_seqcount_begin() and write_seqcount_end() while sk_uid_seq.sequence is
odd and calls sk_uid(), read_seqcount_begin(&sk->sk_uid_seq) will spin
indefinitely in __read_seqcount_begin():

include/linux/seqlock.h:__read_seqcount_begin() {
	while (unlikely((__seq = seqprop_sequence(s)) & 1))
		cpu_relax();
}

With a raw seqcount_t, the reader has no associated rwlock_t to block on and
priority-boost the preempted writer. Should sk_uid_seq be a
seqcount_rwlock_t initialized with sk_callback_lock, or should preemption be
disabled around the write section?

> diff --git a/net/core/sock.c b/net/core/sock.c
> index e8551df8330ff..300bc75ce13e6 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c

[ ... ]

> @@ -3773,7 +3776,7 @@ void sock_init_data_uid(struct socket *sock, struct sock *sk, kuid_t uid)
>  	} else {
>  		RCU_INIT_POINTER(sk->sk_wq, NULL);
>  	}
> -	sk->sk_uid	=	uid;
> +	__sk_set_uid(sk, uid);
>  
>  	sk->sk_state_change	=	sock_def_wakeup;
>  	sk->sk_data_ready	=	sock_def_readable;

[Severity: High]
What happens when sock_init_data_uid() calls sk_set_socket() and
__sk_set_uid() in preemptible process context with softirqs enabled?

During socket creation (for example, inet_create() -> sock_init_data() ->
sock_init_data_uid()), sock_init_data_uid() runs in normal process context
with preemption and bottom halves enabled and no locks held. Calling
__sk_set_uid() (both via sk_set_socket() when sock is non-null and directly
in sock_init_data_uid()) invokes write_seqcount_begin(&sk->sk_uid_seq) on a
raw seqcount_t while preemption is enabled, which immediately triggers the
warning in lockdep_assert_preemption_disabled().

Furthermore, write_seqcount_begin(&sk->sk_uid_seq) records a lockdep write
acquisition on the sk_uid_seq class with softirqs enabled. When sk_uid() is
later called from softirq context (such as during tcp_v4_rcv() ->
tcp_v4_syn_recv_sock() -> sk_clone(), or during routing and reuseport
lookups), read_seqcount_begin(&sk->sk_uid_seq) records a softirq read on the
same class in mark_usage() in kernel/locking/lockdep.c, triggering an
inconsistent lock state warning (softirq-on-w to in-softirq-r).

Since the socket is not yet published in sock_init_data_uid(), should
sk->sk_uid be initialized with a plain assignment when the socket is still
unshared, only using __sk_set_uid() under write_lock_bh(&sk->sk_callback_lock)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=4