| Message ID | 20211105014953.972946-2-dima@arista.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp/md5: Generic tcp_sig_pool | expand |
On 11/4/21 6:49 PM, Dmitry Safonov wrote: > static_branch_unlikely(&tcp_md5_needed) is enabled by > tcp_alloc_md5sig_pool(), so as long as the code doesn't change > tcp_md5sig_pool has been already populated if this code is being > executed. > > In case tcptw->tw_md5_key allocaion failed - no reason to crash kernel: > tcp_{v4,v6}_send_ack() will send unsigned segment, the connection won't be > established, which is bad enough, but in OOM situation totally > acceptable and better than kernel crash. > > Introduce tcp_md5sig_pool_ready() helper. > tcp_alloc_md5sig_pool() usage is intentionally avoided here as it's > fast-path here and it's check for sanity rather than point of actual > pool allocation. That will allow to have generic slow-path allocator > for tcp crypto pool. > > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- > include/net/tcp.h | 1 + > net/ipv4/tcp.c | 5 +++++ > net/ipv4/tcp_minisocks.c | 5 +++-- > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 4da22b41bde6..3e5423a10a74 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1672,6 +1672,7 @@ tcp_md5_do_lookup(const struct sock *sk, int l3index, > #endif > > bool tcp_alloc_md5sig_pool(void); > +bool tcp_md5sig_pool_ready(void); > > struct tcp_md5sig_pool *tcp_get_md5sig_pool(void); > static inline void tcp_put_md5sig_pool(void) > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index b7796b4cf0a0..c0856a6af9f5 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4314,6 +4314,11 @@ bool tcp_alloc_md5sig_pool(void) > } > EXPORT_SYMBOL(tcp_alloc_md5sig_pool); > > +bool tcp_md5sig_pool_ready(void) > +{ > + return tcp_md5sig_pool_populated; > +} > +EXPORT_SYMBOL(tcp_md5sig_pool_ready); > > /** > * tcp_get_md5sig_pool - get md5sig_pool for this user > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > index cf913a66df17..c99cdb529902 100644 > --- a/net/ipv4/tcp_minisocks.c > +++ b/net/ipv4/tcp_minisocks.c > @@ -293,11 +293,12 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) > tcptw->tw_md5_key = NULL; > if (static_branch_unlikely(&tcp_md5_needed)) { > struct tcp_md5sig_key *key; > + bool err = WARN_ON(!tcp_md5sig_pool_ready()); > > key = tp->af_specific->md5_lookup(sk, sk); > - if (key) { > + if (key && !err) { > tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC); > - BUG_ON(tcptw->tw_md5_key && !tcp_alloc_md5sig_pool()); > + WARN_ON_ONCE(tcptw->tw_md5_key == NULL); > } > } > } while (0); > Hmmm.... how this BUG_ON() could trigger exactly ? tcp_md5_needed can only be enabled after __tcp_alloc_md5sig_pool has succeeded. This patch, sent during merge-window, is a distraction, sorry. About renaming : It looks nice, but is a disaster for backports done for stable releases. Please refrain from doing this.
On 11/5/21 4:55 AM, Eric Dumazet wrote: > > > On 11/4/21 6:49 PM, Dmitry Safonov wrote: >> static_branch_unlikely(&tcp_md5_needed) is enabled by >> tcp_alloc_md5sig_pool(), so as long as the code doesn't change >> tcp_md5sig_pool has been already populated if this code is being >> executed. >> >> In case tcptw->tw_md5_key allocaion failed - no reason to crash kernel: >> tcp_{v4,v6}_send_ack() will send unsigned segment, the connection won't be >> established, which is bad enough, but in OOM situation totally >> acceptable and better than kernel crash. >> >> Introduce tcp_md5sig_pool_ready() helper. >> tcp_alloc_md5sig_pool() usage is intentionally avoided here as it's >> fast-path here and it's check for sanity rather than point of actual >> pool allocation. That will allow to have generic slow-path allocator >> for tcp crypto pool. >> >> Signed-off-by: Dmitry Safonov <dima@arista.com> >> --- >> include/net/tcp.h | 1 + >> net/ipv4/tcp.c | 5 +++++ >> net/ipv4/tcp_minisocks.c | 5 +++-- >> 3 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 4da22b41bde6..3e5423a10a74 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1672,6 +1672,7 @@ tcp_md5_do_lookup(const struct sock *sk, int l3index, >> #endif >> >> bool tcp_alloc_md5sig_pool(void); >> +bool tcp_md5sig_pool_ready(void); >> >> struct tcp_md5sig_pool *tcp_get_md5sig_pool(void); >> static inline void tcp_put_md5sig_pool(void) >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index b7796b4cf0a0..c0856a6af9f5 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -4314,6 +4314,11 @@ bool tcp_alloc_md5sig_pool(void) >> } >> EXPORT_SYMBOL(tcp_alloc_md5sig_pool); >> >> +bool tcp_md5sig_pool_ready(void) >> +{ >> + return tcp_md5sig_pool_populated; >> +} >> +EXPORT_SYMBOL(tcp_md5sig_pool_ready); >> >> /** >> * tcp_get_md5sig_pool - get md5sig_pool for this user >> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c >> index cf913a66df17..c99cdb529902 100644 >> --- a/net/ipv4/tcp_minisocks.c >> +++ b/net/ipv4/tcp_minisocks.c >> @@ -293,11 +293,12 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) >> tcptw->tw_md5_key = NULL; >> if (static_branch_unlikely(&tcp_md5_needed)) { >> struct tcp_md5sig_key *key; >> + bool err = WARN_ON(!tcp_md5sig_pool_ready()); >> >> key = tp->af_specific->md5_lookup(sk, sk); >> - if (key) { >> + if (key && !err) { >> tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC); >> - BUG_ON(tcptw->tw_md5_key && !tcp_alloc_md5sig_pool()); >> + WARN_ON_ONCE(tcptw->tw_md5_key == NULL); >> } >> } >> } while (0); >> > > Hmmm.... how this BUG_ON() could trigger exactly ? > > tcp_md5_needed can only be enabled after __tcp_alloc_md5sig_pool has succeeded. The tcp_alloc_md5_pool here should just be removed, it no longer does anything since md5 pool reference counting and freeing were removed back in 2013 by commit 71cea17ed39f ("tcp: md5: remove spinlock usage in fast path").
On 11/5/21 02:55, Eric Dumazet wrote: > > > On 11/4/21 6:49 PM, Dmitry Safonov wrote: >> static_branch_unlikely(&tcp_md5_needed) is enabled by >> tcp_alloc_md5sig_pool(), so as long as the code doesn't change >> tcp_md5sig_pool has been already populated if this code is being >> executed. >> >> In case tcptw->tw_md5_key allocaion failed - no reason to crash kernel: >> tcp_{v4,v6}_send_ack() will send unsigned segment, the connection won't be >> established, which is bad enough, but in OOM situation totally >> acceptable and better than kernel crash. >> >> Introduce tcp_md5sig_pool_ready() helper. >> tcp_alloc_md5sig_pool() usage is intentionally avoided here as it's >> fast-path here and it's check for sanity rather than point of actual >> pool allocation. That will allow to have generic slow-path allocator >> for tcp crypto pool. >> >> Signed-off-by: Dmitry Safonov <dima@arista.com> >> --- >> include/net/tcp.h | 1 + >> net/ipv4/tcp.c | 5 +++++ >> net/ipv4/tcp_minisocks.c | 5 +++-- >> 3 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/include/net/tcp.h b/include/net/tcp.h >> index 4da22b41bde6..3e5423a10a74 100644 >> --- a/include/net/tcp.h >> +++ b/include/net/tcp.h >> @@ -1672,6 +1672,7 @@ tcp_md5_do_lookup(const struct sock *sk, int l3index, >> #endif >> >> bool tcp_alloc_md5sig_pool(void); >> +bool tcp_md5sig_pool_ready(void); >> >> struct tcp_md5sig_pool *tcp_get_md5sig_pool(void); >> static inline void tcp_put_md5sig_pool(void) >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index b7796b4cf0a0..c0856a6af9f5 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -4314,6 +4314,11 @@ bool tcp_alloc_md5sig_pool(void) >> } >> EXPORT_SYMBOL(tcp_alloc_md5sig_pool); >> >> +bool tcp_md5sig_pool_ready(void) >> +{ >> + return tcp_md5sig_pool_populated; >> +} >> +EXPORT_SYMBOL(tcp_md5sig_pool_ready); >> >> /** >> * tcp_get_md5sig_pool - get md5sig_pool for this user >> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c >> index cf913a66df17..c99cdb529902 100644 >> --- a/net/ipv4/tcp_minisocks.c >> +++ b/net/ipv4/tcp_minisocks.c >> @@ -293,11 +293,12 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) >> tcptw->tw_md5_key = NULL; >> if (static_branch_unlikely(&tcp_md5_needed)) { >> struct tcp_md5sig_key *key; >> + bool err = WARN_ON(!tcp_md5sig_pool_ready()); >> >> key = tp->af_specific->md5_lookup(sk, sk); >> - if (key) { >> + if (key && !err) { >> tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC); >> - BUG_ON(tcptw->tw_md5_key && !tcp_alloc_md5sig_pool()); >> + WARN_ON_ONCE(tcptw->tw_md5_key == NULL); >> } >> } >> } while (0); >> > > Hmmm.... how this BUG_ON() could trigger exactly ? > > tcp_md5_needed can only be enabled after __tcp_alloc_md5sig_pool has succeeded. Yeah, I've misread this part as : BUG_ON(!tcptw->tw_md5_key || !tcp_alloc_md5sig_pool()); Still, there is an issue with checking tcp_alloc_md5sig_pool(): currently the condition is never true, but if it ever becomes true, the tcp_alloc_md5sig_pool() call may cause tcp_time_wait() to sleep with bh disabled (i.e. __tcp_close()). So, if this condition ever becomes true, it will cause an issue checking it here. I'll squash this with patch 3 and send when the merge window closes. Thanks, Dmitry
diff --git a/include/net/tcp.h b/include/net/tcp.h index 4da22b41bde6..3e5423a10a74 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1672,6 +1672,7 @@ tcp_md5_do_lookup(const struct sock *sk, int l3index, #endif bool tcp_alloc_md5sig_pool(void); +bool tcp_md5sig_pool_ready(void); struct tcp_md5sig_pool *tcp_get_md5sig_pool(void); static inline void tcp_put_md5sig_pool(void) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index b7796b4cf0a0..c0856a6af9f5 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4314,6 +4314,11 @@ bool tcp_alloc_md5sig_pool(void) } EXPORT_SYMBOL(tcp_alloc_md5sig_pool); +bool tcp_md5sig_pool_ready(void) +{ + return tcp_md5sig_pool_populated; +} +EXPORT_SYMBOL(tcp_md5sig_pool_ready); /** * tcp_get_md5sig_pool - get md5sig_pool for this user diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index cf913a66df17..c99cdb529902 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -293,11 +293,12 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) tcptw->tw_md5_key = NULL; if (static_branch_unlikely(&tcp_md5_needed)) { struct tcp_md5sig_key *key; + bool err = WARN_ON(!tcp_md5sig_pool_ready()); key = tp->af_specific->md5_lookup(sk, sk); - if (key) { + if (key && !err) { tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC); - BUG_ON(tcptw->tw_md5_key && !tcp_alloc_md5sig_pool()); + WARN_ON_ONCE(tcptw->tw_md5_key == NULL); } } } while (0);
static_branch_unlikely(&tcp_md5_needed) is enabled by tcp_alloc_md5sig_pool(), so as long as the code doesn't change tcp_md5sig_pool has been already populated if this code is being executed. In case tcptw->tw_md5_key allocaion failed - no reason to crash kernel: tcp_{v4,v6}_send_ack() will send unsigned segment, the connection won't be established, which is bad enough, but in OOM situation totally acceptable and better than kernel crash. Introduce tcp_md5sig_pool_ready() helper. tcp_alloc_md5sig_pool() usage is intentionally avoided here as it's fast-path here and it's check for sanity rather than point of actual pool allocation. That will allow to have generic slow-path allocator for tcp crypto pool. Signed-off-by: Dmitry Safonov <dima@arista.com> --- include/net/tcp.h | 1 + net/ipv4/tcp.c | 5 +++++ net/ipv4/tcp_minisocks.c | 5 +++-- 3 files changed, 9 insertions(+), 2 deletions(-)