Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control

[Eric Dumazet]

On Mon, Sep 23, 2024 at 5:16 AM Jiawei Ye <jiawei.ye@xxxxxxxxxxx> wrote:
>
> Thank you very much for your feedback, Florian Westphal and Eric Dumazet.
>
> On 9/20/24 22:11, Eric Dumazet wrote
> >
> > On Fri, Sep 20, 2024 at 11:35 AM Florian Westphal <fw@xxxxxxxxx> wrote:
> > >
> > > Jiawei Ye <jiawei.ye@xxxxxxxxxxx> wrote:
> > > > In the `tcp_assign_congestion_control` function, the `ca->flags` is
> > > > accessed after the RCU read-side critical section is unlocked. According
> > > > to RCU usage rules, this is illegal. Reusing this pointer can lead to
> > > > unpredictable behavior, including accessing memory that has been updated
> > > > or causing use-after-free issues.
> > > >
> > > > This possible bug was identified using a static analysis tool developed
> > > > by myself, specifically designed to detect RCU-related issues.
> > > >
> > > > To resolve this issue, the `rcu_read_unlock` call has been moved to the
> > > > end of the function.
> > > >
> > > > Signed-off-by: Jiawei Ye <jiawei.ye@xxxxxxxxxxx>
> > > > ---
> > > > In another part of the file, `tcp_set_congestion_control` calls
> > > > `tcp_reinit_congestion_control`, ensuring that the congestion control
> > > > reinitialization process is protected by RCU. The
> > > > `tcp_reinit_congestion_control` function contains operations almost
> > > > identical to those in `tcp_assign_congestion_control`, but the former
> > > > operates under full RCU protection, whereas the latter is only partially
> > > > protected. The differing protection strategies between the two may
> > > > warrant further unification.
> > > > ---
> > > >  net/ipv4/tcp_cong.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> > > > index 0306d257fa64..356a59d316e3 100644
> > > > --- a/net/ipv4/tcp_cong.c
> > > > +++ b/net/ipv4/tcp_cong.c
> > > > @@ -223,13 +223,13 @@ void tcp_assign_congestion_control(struct sock *sk)
> > > >       if (unlikely(!bpf_try_module_get(ca, ca->owner)))
> > > >               ca = &tcp_reno;
> > >
> > > After this, ca either has module refcount incremented, so it can't
> > > go away anymore, or reno fallback was enabled (always bultin).
> > >
> > > >       icsk->icsk_ca_ops = ca;
> > > > -     rcu_read_unlock();
> > >
> > > Therefore its ok to rcu unlock here.
> >
> > I agree, there is no bug here.
> >
> > Jiawei Ye, I guess your static analysis tool is not ready yet.
>
> Yes, the static analysis tool is still under development and debugging.
>
> While I've collected and analyzed some relevant RCU commits from
> associated repositories, designing an effective static detection tool
> remains challenging.
>
> It's quite difficult without the assistance of experienced developers. If
> you have any suggestions or examples, I would greatly appreciate your
> help.
>

This case is explained in Documentation/RCU/rcuref.rst

line 61 : search_and_reference()

For congestion control modules, we call try_module_get() which calls
atomic_inc_not_zero(&module->refcnt)