[PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[Breno Leitao]

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();
+	ret = mptcp_init_sched(mptcp_sk(sk), sched);
 	if (ret)
 		return ret;
 
-- 
2.43.5

Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[Eric Dumazet]

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
>

Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[Breno Leitao]

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.

Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[Eric Dumazet]

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.

Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[Matthieu Baerts]

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

Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

[Breno Leitao]

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