Hello Eric,
On Wed, Oct 30, 2024 at 03:18:14PM +0100, Eric Dumazet wrote:
On Wed, Oct 30, 2024 at 3:02 PM Breno Leitao leitao@debian.org wrote:
The mptcp_sched_find() function must be called with the RCU read lock held, as it accesses RCU-protected data structures. This requirement was not properly enforced in the mptcp_init_sock() function, leading to a RCU list traversal in a non-reader section error when CONFIG_PROVE_RCU_LIST is enabled.
net/mptcp/sched.c:44 RCU-list traversed in non-reader section!!Fix it by acquiring the RCU read lock before calling the mptcp_sched_find() function. This ensures that the function is invoked with the necessary RCU protection in place, as it accesses RCU-protected data structures.
Additionally, the patch breaks down the mptcp_init_sched() call into smaller parts, with the RCU read lock only covering the specific call to mptcp_sched_find(). This helps minimize the critical section, reducing the time during which RCU grace periods are blocked.
The mptcp_sched_list_lock is not held in this case, and it is not clear if it is necessary.
Signed-off-by: Breno Leitao leitao@debian.org Fixes: 1730b2b2c5a5 ("mptcp: add sched in mptcp_sock") Cc: stable@vger.kernel.org
net/mptcp/protocol.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 6d0e201c3eb2..8ece630f80d4 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -2854,6 +2854,7 @@ static void mptcp_ca_reset(struct sock *sk) static int mptcp_init_sock(struct sock *sk) { struct net *net = sock_net(sk);
struct mptcp_sched_ops *sched; int ret; __mptcp_init_sock(sk);@@ -2864,8 +2865,10 @@ static int mptcp_init_sock(struct sock *sk) if (unlikely(!net->mib.mptcp_statistics) && !mptcp_mib_alloc(net)) return -ENOMEM;
ret = mptcp_init_sched(mptcp_sk(sk),mptcp_sched_find(mptcp_get_scheduler(net)));
rcu_read_lock();sched = mptcp_sched_find(mptcp_get_scheduler(net));rcu_read_unlock();You are silencing the warning, but a potential UAF remains.
sched could have been freed already, it is illegal to deref it.
Thanks. I got the impression that the scheduler list was append-only, and the entries were never freed.