[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241030-keen-vicugna-of-effort-bd3ab8@leitao>
Date: Wed, 30 Oct 2024 08:05:16 -0700
From: Breno Leitao <leitao@...ian.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: kuba@...nel.org, horms@...nel.org, davem@...emloft.net,
pabeni@...hat.com, Matthieu Baerts <matttbe@...nel.org>,
Mat Martineau <martineau@...nel.org>,
Geliang Tang <geliang@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, vlad.wing@...il.com, max@...sevol.com,
kernel-team@...a.com, aehkn@...hub.one, stable@...r.kernel.org,
"open list:NETWORKING [MPTCP]" <mptcp@...ts.linux.dev>
Subject: Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling
mptcp_sched_find()
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@...ian.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@...ian.org>
> > Fixes: 1730b2b2c5a5 ("mptcp: add sched in mptcp_sock")
> > Cc: stable@...r.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.
Powered by blists - more mailing lists