netdev - Re: BUG: using __this_cpu_add() in preemptible in tcp_make

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Feb 2023 20:59:29 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Frank Hofmann <fhofmann@...udflare.com>
Cc:     Frederick Lawler <fred@...udflare.com>, netdev@...r.kernel.org,
        kernel-team@...udflare.com, davem@...emloft.net,
        yoshfuji@...ux-ipv6.org, dsahern@...nel.org, kuba@...nel.org,
        pabeni@...hat.com
Subject: Re: BUG: using __this_cpu_add() in preemptible in tcp_make_synack()

On Tue, Feb 14, 2023 at 6:14 PM Frank Hofmann <fhofmann@...udflare.com> wrote:
>
> Hi Eric,
>
> On Mon, Jan 23, 2023 at 3:49 PM 'Eric Dumazet' via
> kernel-team+notifications <kernel-team@...udflare.com> wrote:
> >
> > > On 1/18/23 11:07 AM, Eric Dumazet wrote:
> > [ ... ]
> > > > Thanks for the report
> > > >
> > > > I guess this part has been missed in commit 0a375c822497ed6a
> > > >
> > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > > > index 71d01cf3c13eb4bd3d314ef140568d2ffd6a499e..ba839e441450f195012a8d77cb9e5ed956962d2f
> > > > 100644
> > > > --- a/net/ipv4/tcp_output.c
> > > > +++ b/net/ipv4/tcp_output.c
> > > > @@ -3605,7 +3605,7 @@ struct sk_buff *tcp_make_synack(const struct
> > > > sock *sk, struct dst_entry *dst,
> [ ... ]
>
> we're still seeing this with a preempt-enabled kernel, in
> tcp_check_req() though, like:
>
> BUG: using __this_cpu_add() in preemptible [00000000] code: nginx-ssl/186233
> caller is tcp_check_req+0x49a/0x660
> CPU: 58 PID: 186233 Comm: nginx-ssl Kdump: loaded Tainted: G
> O       6.1.8-cloudflare-2023.1.16 #1
> Hardware name: ...
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x34/0x48
>  check_preemption_disabled+0xdd/0xe0
>  tcp_check_req+0x49a/0x660
>  tcp_rcv_state_process+0xa3/0x1020
>  ? tcp_sendmsg_locked+0x2a4/0xc50
>  tcp_v4_do_rcv+0xc6/0x280
>  __release_sock+0xb4/0xc0
>  release_sock+0x2b/0x90
>  tcp_sendmsg+0x33/0x40
>  sock_sendmsg+0x5b/0x70
>  sock_write_iter+0x97/0x100
>  vfs_write+0x330/0x3d0
>  ksys_write+0xab/0xe0
>  ? syscall_trace_enter.constprop.0+0x164/0x170
>  do_syscall_64+0x3b/0x90
>  entry_SYSCALL_64_after_hwframe+0x4b/0xb5
>
> There's a notable number of "__"-marked stats updates in
> tcp_check_req(); I can't claim to understand the code well enough if
> all would have to be changed.
> The occurence is infrequent (we see about two a week).
>
> Thanks for any pointers!
> Frank Hofmann

Thanks for the report.

I think the following patch should help, please let me know if any
more issues are detected.

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index e002f2e1d4f2de0397f2cc7ec0a14a05efbd802b..9a7ef7732c24c94d4a01d5911ebe51f21371a457
100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -597,6 +597,9 @@ EXPORT_SYMBOL(tcp_create_openreq_child);
  * validation and inside tcp_v4_reqsk_send_ack(). Can we do better?
  *
  * We don't need to initialize tmp_opt.sack_ok as we don't use the results
+ *
+ * Note: If @fastopen is true, this can be called from process or BH context.
+ *       Otherwise, this is only called from BH context.
  */

 struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
@@ -748,7 +751,7 @@ struct sock *tcp_check_req(struct sock *sk, struct
sk_buff *skb,
                                          &tcp_rsk(req)->last_oow_ack_time))
                        req->rsk_ops->send_ack(sk, skb, req);
                if (paws_reject)
-                       __NET_INC_STATS(sock_net(sk),
LINUX_MIB_PAWSESTABREJECTED);
+                       NET_INC_STATS(sock_net(sk),
LINUX_MIB_PAWSESTABREJECTED);
                return NULL;
        }

@@ -767,7 +770,7 @@ struct sock *tcp_check_req(struct sock *sk, struct
sk_buff *skb,
         *         "fourth, check the SYN bit"
         */
        if (flg & (TCP_FLAG_RST|TCP_FLAG_SYN)) {
-               __TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);
+               TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);
                goto embryonic_reset;
        }