| Message ID | 20221202052847.2623997-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 55fb80d518c7323d05b71eda0c9f9d657b373816 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tcp: use 2-arg optimal variant of kfree_rcu() | expand |
On Fri, Dec 2, 2022 at 10:59 AM Eric Dumazet <edumazet@google.com> wrote: > > kfree_rcu(1-arg) should be avoided as much as possible, > since this is only possible from sleepable contexts, > and incurr extra rcu barriers. > > I wish the 1-arg variant of kfree_rcu() would > get a distinct name, like kfree_rcu_slow() > to avoid it being abused. > > Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: Paul E. McKenney <paulmck@kernel.org> > --- > net/ipv4/tcp_ipv4.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 7fae586405cfb10011a0674289280bf400dfa8d8..8320d0ecb13ae1e3e259f3c13a4c2797fbd984a5 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1245,7 +1245,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > > md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); > rcu_assign_pointer(tp->md5sig_info, NULL); > - kfree_rcu(md5sig); > + kfree_rcu(md5sig, rcu); > return -EUSERS; > } > } > @@ -1271,7 +1271,7 @@ int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, > md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); > net_warn_ratelimited("Too many TCP-MD5 keys in the system\n"); > rcu_assign_pointer(tp->md5sig_info, NULL); > - kfree_rcu(md5sig); > + kfree_rcu(md5sig, rcu); > return -EUSERS; > } > } > -- > 2.39.0.rc0.267.gcb52ba06e7-goog > Looks good to me. Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
On 12/2/22 05:28, Eric Dumazet wrote: > kfree_rcu(1-arg) should be avoided as much as possible, > since this is only possible from sleepable contexts, > and incurr extra rcu barriers. > > I wish the 1-arg variant of kfree_rcu() would > get a distinct name, like kfree_rcu_slow() > to avoid it being abused. Thanks again! > Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: Paul E. McKenney <paulmck@kernel.org> Reviewed-by: Dmitry Safonov <dima@arista.com> > --- > net/ipv4/tcp_ipv4.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 7fae586405cfb10011a0674289280bf400dfa8d8..8320d0ecb13ae1e3e259f3c13a4c2797fbd984a5 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1245,7 +1245,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > > md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); > rcu_assign_pointer(tp->md5sig_info, NULL); > - kfree_rcu(md5sig); > + kfree_rcu(md5sig, rcu); > return -EUSERS; > } > } > @@ -1271,7 +1271,7 @@ int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, > md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); > net_warn_ratelimited("Too many TCP-MD5 keys in the system\n"); > rcu_assign_pointer(tp->md5sig_info, NULL); > - kfree_rcu(md5sig); > + kfree_rcu(md5sig, rcu); > return -EUSERS; > } > }
On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > kfree_rcu(1-arg) should be avoided as much as possible, > since this is only possible from sleepable contexts, > and incurr extra rcu barriers. > > I wish the 1-arg variant of kfree_rcu() would > get a distinct name, like kfree_rcu_slow() > to avoid it being abused. Fair point, the bug of forgetting the second argument goes unflagged, at least from contexts where blocking is OK. Let the bikeshedding commence! ;-) Thanx, Paul > Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: Paul E. McKenney <paulmck@kernel.org> > --- > net/ipv4/tcp_ipv4.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 7fae586405cfb10011a0674289280bf400dfa8d8..8320d0ecb13ae1e3e259f3c13a4c2797fbd984a5 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1245,7 +1245,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, > > md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); > rcu_assign_pointer(tp->md5sig_info, NULL); > - kfree_rcu(md5sig); > + kfree_rcu(md5sig, rcu); > return -EUSERS; > } > } > @@ -1271,7 +1271,7 @@ int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, > md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); > net_warn_ratelimited("Too many TCP-MD5 keys in the system\n"); > rcu_assign_pointer(tp->md5sig_info, NULL); > - kfree_rcu(md5sig); > + kfree_rcu(md5sig, rcu); > return -EUSERS; > } > } > -- > 2.39.0.rc0.267.gcb52ba06e7-goog >
On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > kfree_rcu(1-arg) should be avoided as much as possible, > since this is only possible from sleepable contexts, > and incurr extra rcu barriers. > > I wish the 1-arg variant of kfree_rcu() would > get a distinct name, like kfree_rcu_slow() > to avoid it being abused. Hi Eric, Nice to see your patch. Paul, all, regarding Eric's concern, would the following work to warn of users? Credit to Paul/others for discussing the idea on another thread. One thing to note here is, this debugging will only be in effect on preemptible kernels, but should still help catch issues hopefully. The other idea Paul mentioned is to introduce a new dedicated API for 1-arg sleepable cases. My concern with that was that, that being effective depends on the user using the right API in the first place. I did not test it yet, but wanted to discuss a bit first. Cheers, - Joel ---8<----------------------- diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h index 9bc025aa79a3..112d230279ea 100644 --- a/include/linux/rcutiny.h +++ b/include/linux/rcutiny.h @@ -106,6 +106,11 @@ static inline void __kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) } // kvfree_rcu(one_arg) call. + if (IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible() && !head) { + WARN_ONCE(1, "%s(): Please provide an rcu_head in preemptible" + " contexts to avoid long waits!\n", __func__); + } + might_sleep(); synchronize_rcu(); kvfree((void *) func); diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 0ca21ac0f064..b29df1305a2e 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3324,6 +3324,11 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) * only. For other places please embed an rcu_head to * your data. */ + if (IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible() && !head) { + WARN_ONCE(1, "%s(): Please provide an rcu_head in preemptible" + " contexts to avoid long waits!\n", __func__); + } + might_sleep(); ptr = (unsigned long *) func; }
On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: > On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > > kfree_rcu(1-arg) should be avoided as much as possible, > > since this is only possible from sleepable contexts, > > and incurr extra rcu barriers. > > > > I wish the 1-arg variant of kfree_rcu() would > > get a distinct name, like kfree_rcu_slow() > > to avoid it being abused. > > Hi Eric, > Nice to see your patch. > > Paul, all, regarding Eric's concern, would the following work to warn of > users? Credit to Paul/others for discussing the idea on another thread. One > thing to note here is, this debugging will only be in effect on preemptible > kernels, but should still help catch issues hopefully. Mightn't there be some places where someone needs to invoke single-argument kfree_rcu() in a preemptible context, for example, due to the RCU-protected structure being very small and very numerous? > The other idea Paul mentioned is to introduce a new dedicated API for 1-arg > sleepable cases. My concern with that was that, that being effective depends > on the user using the right API in the first place. Actually, Eric's idea from above. ;-) Thanx, Paul > I did not test it yet, but wanted to discuss a bit first. > > Cheers, > > - Joel > > ---8<----------------------- > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > index 9bc025aa79a3..112d230279ea 100644 > --- a/include/linux/rcutiny.h > +++ b/include/linux/rcutiny.h > @@ -106,6 +106,11 @@ static inline void __kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > } > > // kvfree_rcu(one_arg) call. > + if (IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible() && !head) { > + WARN_ONCE(1, "%s(): Please provide an rcu_head in preemptible" > + " contexts to avoid long waits!\n", __func__); > + } > + > might_sleep(); > synchronize_rcu(); > kvfree((void *) func); > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 0ca21ac0f064..b29df1305a2e 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3324,6 +3324,11 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > * only. For other places please embed an rcu_head to > * your data. > */ > + if (IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible() && !head) { > + WARN_ONCE(1, "%s(): Please provide an rcu_head in preemptible" > + " contexts to avoid long waits!\n", __func__); > + } > + > might_sleep(); > ptr = (unsigned long *) func; > }
On Sat, Dec 3, 2022 at 12:03 AM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: > > On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > > > kfree_rcu(1-arg) should be avoided as much as possible, > > > since this is only possible from sleepable contexts, > > > and incurr extra rcu barriers. > > > > > > I wish the 1-arg variant of kfree_rcu() would > > > get a distinct name, like kfree_rcu_slow() > > > to avoid it being abused. > > > > Hi Eric, > > Nice to see your patch. > > > > Paul, all, regarding Eric's concern, would the following work to warn of > > users? Credit to Paul/others for discussing the idea on another thread. One > > thing to note here is, this debugging will only be in effect on preemptible > > kernels, but should still help catch issues hopefully. > > Mightn't there be some places where someone needs to invoke > single-argument kfree_rcu() in a preemptible context, for example, > due to the RCU-protected structure being very small and very numerous? This could be possible but I am not able to find examples of such cases, at the moment. Another approach could be to introduce a dedicated API for such cases, where the warning will not fire. And keep the warning otherwise. Example: kfree_rcu_headless() With a big comment saying, use only if you are calling from a preemptible context and cannot absolutely embed an rcu_head. :-) Thoughts? Cheers, - Joel
On Sat, Dec 3, 2022 at 12:12 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Sat, Dec 3, 2022 at 12:03 AM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: > > > On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > > > > kfree_rcu(1-arg) should be avoided as much as possible, > > > > since this is only possible from sleepable contexts, > > > > and incurr extra rcu barriers. > > > > > > > > I wish the 1-arg variant of kfree_rcu() would > > > > get a distinct name, like kfree_rcu_slow() > > > > to avoid it being abused. > > > > > > Hi Eric, > > > Nice to see your patch. > > > > > > Paul, all, regarding Eric's concern, would the following work to warn of > > > users? Credit to Paul/others for discussing the idea on another thread. One > > > thing to note here is, this debugging will only be in effect on preemptible > > > kernels, but should still help catch issues hopefully. > > > > Mightn't there be some places where someone needs to invoke > > single-argument kfree_rcu() in a preemptible context, for example, > > due to the RCU-protected structure being very small and very numerous? > > This could be possible but I am not able to find examples of such > cases, at the moment. Another approach could be to introduce a > dedicated API for such cases, where the warning will not fire. And > keep the warning otherwise. > > Example: kfree_rcu_headless() > With a big comment saying, use only if you are calling from a > preemptible context and cannot absolutely embed an rcu_head. :-) > > Thoughts? > Just to clarify, where I was getting at was to combine both ideas: 1. new API with suppression of the new warning mentioned above. 2. old API but add new warning mentioned above. Cheers, - Joel
+rcu for archives > On Dec 2, 2022, at 7:16 PM, Joel Fernandes <joel@joelfernandes.org> wrote: > > On Sat, Dec 3, 2022 at 12:12 AM Joel Fernandes <joel@joelfernandes.org> wrote: >> >>> On Sat, Dec 3, 2022 at 12:03 AM Paul E. McKenney <paulmck@kernel.org> wrote: >>> >>> On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: >>>> On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: >>>>> kfree_rcu(1-arg) should be avoided as much as possible, >>>>> since this is only possible from sleepable contexts, >>>>> and incurr extra rcu barriers. >>>>> >>>>> I wish the 1-arg variant of kfree_rcu() would >>>>> get a distinct name, like kfree_rcu_slow() >>>>> to avoid it being abused. >>>> >>>> Hi Eric, >>>> Nice to see your patch. >>>> >>>> Paul, all, regarding Eric's concern, would the following work to warn of >>>> users? Credit to Paul/others for discussing the idea on another thread. One >>>> thing to note here is, this debugging will only be in effect on preemptible >>>> kernels, but should still help catch issues hopefully. >>> >>> Mightn't there be some places where someone needs to invoke >>> single-argument kfree_rcu() in a preemptible context, for example, >>> due to the RCU-protected structure being very small and very numerous? >> >> This could be possible but I am not able to find examples of such >> cases, at the moment. Another approach could be to introduce a >> dedicated API for such cases, where the warning will not fire. And >> keep the warning otherwise. >> >> Example: kfree_rcu_headless() >> With a big comment saying, use only if you are calling from a >> preemptible context and cannot absolutely embed an rcu_head. :-) >> >> Thoughts? >> > > Just to clarify, where I was getting at was to combine both ideas: > 1. new API with suppression of the new warning mentioned above. > 2. old API but add new warning mentioned above. > > Cheers, > > - Joel
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Fri, 2 Dec 2022 05:28:47 +0000 you wrote: > kfree_rcu(1-arg) should be avoided as much as possible, > since this is only possible from sleepable contexts, > and incurr extra rcu barriers. > > I wish the 1-arg variant of kfree_rcu() would > get a distinct name, like kfree_rcu_slow() > to avoid it being abused. > > [...] Here is the summary with links: - [net-next] tcp: use 2-arg optimal variant of kfree_rcu() https://git.kernel.org/netdev/net-next/c/55fb80d518c7 You are awesome, thank you!
Hello, Eric. > +rcu for archives > > > On Dec 2, 2022, at 7:16 PM, Joel Fernandes <joel@joelfernandes.org> wrote: > > > > On Sat, Dec 3, 2022 at 12:12 AM Joel Fernandes <joel@joelfernandes.org> wrote: > >> > >>> On Sat, Dec 3, 2022 at 12:03 AM Paul E. McKenney <paulmck@kernel.org> wrote: > >>> > >>> On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: > >>>> On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > >>>>> kfree_rcu(1-arg) should be avoided as much as possible, > >>>>> since this is only possible from sleepable contexts, > >>>>> and incurr extra rcu barriers. > >>>>> > >>>>> I wish the 1-arg variant of kfree_rcu() would > >>>>> get a distinct name, like kfree_rcu_slow() > >>>>> to avoid it being abused. > <snip> tcp: use 2-arg optimal variant of kfree_rcu() Date: Fri, 2 Dec 2022 05:28:47 +0000 [thread overview] Message-ID: <20221202052847.2623997-1-edumazet@google.com> (raw) kfree_rcu(1-arg) should be avoided as much as possible, since this is only possible from sleepable contexts, and incurr extra rcu barriers. I wish the 1-arg variant of kfree_rcu() would get a distinct name, like kfree_rcu_slow() to avoid it being abused. Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Dmitry Safonov <dima@arista.com> Cc: Paul E. McKenney <paulmck@kernel.org> <snip> Could you please clarify a little bit about why/how have you came up with a patch that you posted with "Fixes" tag? I mean you run into: - performance degrade; - simple typo; - etc. Thank you. -- Uladzislau Rezki
On Mon, Dec 5, 2022 at 12:09 PM Uladzislau Rezki <urezki@gmail.com> wrote: > > Hello, Eric. > > > +rcu for archives > > > > > On Dec 2, 2022, at 7:16 PM, Joel Fernandes <joel@joelfernandes.org> wrote: > > > > > > On Sat, Dec 3, 2022 at 12:12 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > >> > > >>> On Sat, Dec 3, 2022 at 12:03 AM Paul E. McKenney <paulmck@kernel.org> wrote: > > >>> > > >>> On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: > > >>>> On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > > >>>>> kfree_rcu(1-arg) should be avoided as much as possible, > > >>>>> since this is only possible from sleepable contexts, > > >>>>> and incurr extra rcu barriers. > > >>>>> > > >>>>> I wish the 1-arg variant of kfree_rcu() would > > >>>>> get a distinct name, like kfree_rcu_slow() > > >>>>> to avoid it being abused. > > > <snip> > tcp: use 2-arg optimal variant of kfree_rcu() > Date: Fri, 2 Dec 2022 05:28:47 +0000 [thread overview] > Message-ID: <20221202052847.2623997-1-edumazet@google.com> (raw) > > kfree_rcu(1-arg) should be avoided as much as possible, > since this is only possible from sleepable contexts, > and incurr extra rcu barriers. > > I wish the 1-arg variant of kfree_rcu() would > get a distinct name, like kfree_rcu_slow() > to avoid it being abused. > > Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: Paul E. McKenney <paulmck@kernel.org> > <snip> > > Could you please clarify a little bit about why/how have you came > up with a patch that you posted with "Fixes" tag? I mean you run > into: > - performance degrade; > - simple typo; > - etc. Bug was added in the blamed commit, we use Fixes: tag to clearly identify bug origin. tcp_md5_key_copy() is called from softirq context, there is no way it could sleep in synchronize_rcu()
> On Mon, Dec 5, 2022 at 12:09 PM Uladzislau Rezki <urezki@gmail.com> wrote: > > > > Hello, Eric. > > > > > +rcu for archives > > > > > > > On Dec 2, 2022, at 7:16 PM, Joel Fernandes <joel@joelfernandes.org> wrote: > > > > > > > > On Sat, Dec 3, 2022 at 12:12 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > > >> > > > >>> On Sat, Dec 3, 2022 at 12:03 AM Paul E. McKenney <paulmck@kernel.org> wrote: > > > >>> > > > >>> On Fri, Dec 02, 2022 at 11:49:59PM +0000, Joel Fernandes wrote: > > > >>>> On Fri, Dec 02, 2022 at 05:28:47AM +0000, Eric Dumazet wrote: > > > >>>>> kfree_rcu(1-arg) should be avoided as much as possible, > > > >>>>> since this is only possible from sleepable contexts, > > > >>>>> and incurr extra rcu barriers. > > > >>>>> > > > >>>>> I wish the 1-arg variant of kfree_rcu() would > > > >>>>> get a distinct name, like kfree_rcu_slow() > > > >>>>> to avoid it being abused. > > > > > <snip> > > tcp: use 2-arg optimal variant of kfree_rcu() > > Date: Fri, 2 Dec 2022 05:28:47 +0000 [thread overview] > > Message-ID: <20221202052847.2623997-1-edumazet@google.com> (raw) > > > > kfree_rcu(1-arg) should be avoided as much as possible, > > since this is only possible from sleepable contexts, > > and incurr extra rcu barriers. > > > > I wish the 1-arg variant of kfree_rcu() would > > get a distinct name, like kfree_rcu_slow() > > to avoid it being abused. > > > > Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Dmitry Safonov <dima@arista.com> > > Cc: Paul E. McKenney <paulmck@kernel.org> > > <snip> > > > > Could you please clarify a little bit about why/how have you came > > up with a patch that you posted with "Fixes" tag? I mean you run > > into: > > - performance degrade; > > - simple typo; > > - etc. > > Bug was added in the blamed commit, we use Fixes: tag to clearly > identify bug origin. > > tcp_md5_key_copy() is called from softirq context, there is no way it > could sleep in synchronize_rcu() > So it was a typo then. How did you identify that BUG? Simple go through the code? Or some test coverage? Thank you! -- Uladzislau Rezki
On Mon, Dec 5, 2022 at 3:59 PM Uladzislau Rezki <urezki@gmail.com> wrote: > So it was a typo then. How did you identify that BUG? Simple go through > the code? Or some test coverage? Code review. I am the TCP maintainer, in case you do not know. > > Thank you! > > -- > Uladzislau Rezki
> On Mon, Dec 5, 2022 at 3:59 PM Uladzislau Rezki <urezki@gmail.com> wrote: > > > So it was a typo then. How did you identify that BUG? Simple go through > > the code? Or some test coverage? > > Code review. I am the TCP maintainer, in case you do not know. > OK, thank you. -- Uladzislau Rezki
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 7fae586405cfb10011a0674289280bf400dfa8d8..8320d0ecb13ae1e3e259f3c13a4c2797fbd984a5 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1245,7 +1245,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); rcu_assign_pointer(tp->md5sig_info, NULL); - kfree_rcu(md5sig); + kfree_rcu(md5sig, rcu); return -EUSERS; } } @@ -1271,7 +1271,7 @@ int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr, md5sig = rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk)); net_warn_ratelimited("Too many TCP-MD5 keys in the system\n"); rcu_assign_pointer(tp->md5sig_info, NULL); - kfree_rcu(md5sig); + kfree_rcu(md5sig, rcu); return -EUSERS; } }
kfree_rcu(1-arg) should be avoided as much as possible, since this is only possible from sleepable contexts, and incurr extra rcu barriers. I wish the 1-arg variant of kfree_rcu() would get a distinct name, like kfree_rcu_slow() to avoid it being abused. Fixes: 459837b522f7 ("net/tcp: Disable TCP-MD5 static key on tcp_md5sig_info destruction") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Dmitry Safonov <dima@arista.com> Cc: Paul E. McKenney <paulmck@kernel.org> --- net/ipv4/tcp_ipv4.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)