| Message ID | 20241030140224.972565-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find() | expand |
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. > + ret = mptcp_init_sched(mptcp_sk(sk), sched); The rcu_read_unlock() should be moved after this point. This means that mptcp_sched_ops ->init() functions are not allowed to sleep. > if (ret) > return ret; > > -- > 2.43.5 >
Hi Breno 30 Oct 2024 15:02:45 Breno Leitao <leitao@debian.org>: > 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. Thank you for having looked at that, but there is already a fix: https://lore.kernel.org/netdev/20241021-net-mptcp-sched-lock-v1-1-637759cf061c@kernel.org/ This fix has even been applied in the net tree already: https://git.kernel.org/netdev/net/c/3deb12c788c3 Did you not get conflicts when rebasing your branch on top of the latest version? > 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. I agree with Eric (thank you for the review!): this creates other issues. > The mptcp_sched_list_lock is not held in this case, and it is not clear > if it is necessary. It is not needed, the list is not modified, only read with RCU. Cheers, Matt
Hello Matthieu, On Wed, Oct 30, 2024 at 03:45:02PM +0100, Matthieu Baerts wrote: > Hi Breno > > 30 Oct 2024 15:02:45 Breno Leitao <leitao@debian.org>: > > > 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. > > Thank you for having looked at that, but there is already a fix: > > https://lore.kernel.org/netdev/20241021-net-mptcp-sched-lock-v1-1-637759cf061c@kernel.org/ > > This fix has even been applied in the net tree already: > > https://git.kernel.org/netdev/net/c/3deb12c788c3 > > Did you not get conflicts when rebasing your branch on top of the > latest version? Oh, I was testing on Linus' tree when I got the problem, and net was not merged in Linus' tree yet. > > 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. > > I agree with Eric (thank you for the review!): this creates other issues. Let me comment there. > > > The mptcp_sched_list_lock is not held in this case, and it is not clear > > if it is necessary. > > It is not needed, the list is not modified, only read with RCU. Thanks
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.
On Wed, Oct 30, 2024 at 4:05 PM Breno Leitao <leitao@debian.org> wrote: > Thanks. I got the impression that the scheduler list was append-only, > and the entries were never freed. mptcp_unregister_scheduler() is there, and could be used at some point.
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(); + ret = mptcp_init_sched(mptcp_sk(sk), sched); if (ret) return ret;
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(-)