* [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control @ 2024-09-20 9:08 Jiawei Ye 2024-09-20 9:35 ` Florian Westphal 0 siblings, 1 reply; 6+ messages in thread From: Jiawei Ye @ 2024-09-20 9:08 UTC (permalink / raw) To: edumazet, davem, dsahern, kuba, pabeni; +Cc: netdev, linux-kernel 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@foxmail.com> --- 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; icsk->icsk_ca_ops = ca; - rcu_read_unlock(); memset(icsk->icsk_ca_priv, 0, sizeof(icsk->icsk_ca_priv)); if (ca->flags & TCP_CONG_NEEDS_ECN) INET_ECN_xmit(sk); else INET_ECN_dontxmit(sk); + rcu_read_unlock(); } void tcp_init_congestion_control(struct sock *sk) -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control 2024-09-20 9:08 [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control Jiawei Ye @ 2024-09-20 9:35 ` Florian Westphal 2024-09-20 14:11 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Florian Westphal @ 2024-09-20 9:35 UTC (permalink / raw) To: Jiawei Ye; +Cc: edumazet, davem, dsahern, kuba, pabeni, netdev, linux-kernel Jiawei Ye <jiawei.ye@foxmail.com> 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@foxmail.com> > --- > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control 2024-09-20 9:35 ` Florian Westphal @ 2024-09-20 14:11 ` Eric Dumazet 2024-09-23 3:09 ` Jiawei Ye 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2024-09-20 14:11 UTC (permalink / raw) To: Florian Westphal Cc: Jiawei Ye, davem, dsahern, kuba, pabeni, netdev, linux-kernel On Fri, Sep 20, 2024 at 11:35 AM Florian Westphal <fw@strlen.de> wrote: > > Jiawei Ye <jiawei.ye@foxmail.com> 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@foxmail.com> > > --- > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control 2024-09-20 14:11 ` Eric Dumazet @ 2024-09-23 3:09 ` Jiawei Ye 2024-09-23 7:36 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Jiawei Ye @ 2024-09-23 3:09 UTC (permalink / raw) To: edumazet Cc: davem, dsahern, fw, jiawei.ye, kuba, linux-kernel, netdev, pabeni 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@strlen.de> wrote: > > > > Jiawei Ye <jiawei.ye@foxmail.com> 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@foxmail.com> > > > --- > > > 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. Best regards, Jiawei Ye ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control 2024-09-23 3:09 ` Jiawei Ye @ 2024-09-23 7:36 ` Eric Dumazet 2024-09-24 3:57 ` Jiawei Ye 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2024-09-23 7:36 UTC (permalink / raw) To: Jiawei Ye; +Cc: davem, dsahern, fw, kuba, linux-kernel, netdev, pabeni On Mon, Sep 23, 2024 at 5:16 AM Jiawei Ye <jiawei.ye@foxmail.com> 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@strlen.de> wrote: > > > > > > Jiawei Ye <jiawei.ye@foxmail.com> 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@foxmail.com> > > > > --- > > > > 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control 2024-09-23 7:36 ` Eric Dumazet @ 2024-09-24 3:57 ` Jiawei Ye 0 siblings, 0 replies; 6+ messages in thread From: Jiawei Ye @ 2024-09-24 3:57 UTC (permalink / raw) To: edumazet Cc: davem, dsahern, fw, jiawei.ye, kuba, linux-kernel, netdev, pabeni On 9/23/24 15:36, Eric Dumazet wrote: > On Mon, Sep 23, 2024 at 5:16 AM Jiawei Ye <jiawei.ye@foxmail.com> 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@strlen.de> wrote: > > > > > > > > Jiawei Ye <jiawei.ye@foxmail.com> 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@foxmail.com> > > > > > --- > > > > > 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) Thank you, Eric Dumazet. I will further explore the details in Documentation/RCU/rcuref.rst and related documents. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-24 3:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-20 9:08 [PATCH] net: Fix potential RCU dereference issue in tcp_assign_congestion_control Jiawei Ye 2024-09-20 9:35 ` Florian Westphal 2024-09-20 14:11 ` Eric Dumazet 2024-09-23 3:09 ` Jiawei Ye 2024-09-23 7:36 ` Eric Dumazet 2024-09-24 3:57 ` Jiawei Ye
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).