[PATCH bpf-next 0/2] > Keep reference on socket file while wait send memory

[PATCH bpf-next 0/2] > Keep reference on socket file while wait send memory

[Liu Jian]

Keep reference on socket file while wait send memory

Liu Jian (2):
  sk_msg: Keep reference on socket file while wait_memory
  selftests/bpf: Add wait send memory test for sockmap redirect

 net/ipv4/tcp_bpf.c                         |  8 +++++
 tools/testing/selftests/bpf/test_sockmap.c | 42 ++++++++++++++++++++++
 2 files changed, 50 insertions(+)

-- 
2.17.1

[PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[Liu Jian]

Fix the below NULL pointer dereference:

[   14.471200] Call Trace:
[   14.471562]  <TASK>
[   14.471882]  lock_acquire+0x245/0x2e0
[   14.472416]  ? remove_wait_queue+0x12/0x50
[   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
[   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
[   14.474318]  ? remove_wait_queue+0x12/0x50
[   14.474907]  remove_wait_queue+0x12/0x50
[   14.475480]  sk_stream_wait_memory+0x20d/0x340
[   14.476127]  ? do_wait_intr_irq+0x80/0x80
[   14.476704]  do_tcp_sendpages+0x287/0x600
[   14.477283]  tcp_bpf_push+0xab/0x260
[   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
[   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
[   14.479096]  tcp_bpf_send_verdict+0x105/0x470
[   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
[   14.480311]  sock_sendmsg+0x2d/0x40
[   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
[   14.481390]  ? copy_msghdr_from_user+0x62/0x80
[   14.482048]  ___sys_sendmsg+0x78/0xb0
[   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
[   14.483215]  ? __do_fault+0x2a/0x1a0
[   14.483738]  ? do_fault+0x15e/0x5d0
[   14.484246]  ? __handle_mm_fault+0x56b/0x1040
[   14.484874]  ? lock_is_held_type+0xdf/0x130
[   14.485474]  ? find_held_lock+0x2d/0x90
[   14.486046]  ? __sys_sendmsg+0x41/0x70
[   14.486587]  __sys_sendmsg+0x41/0x70
[   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
[   14.487822]  do_syscall_64+0x34/0x80
[   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

The test scene as following flow:
thread1                               thread2
-----------                           ---------------
 tcp_bpf_sendmsg
  tcp_bpf_send_verdict
   tcp_bpf_sendmsg_redir              sock_close
    tcp_bpf_push_locked                 __sock_release
     tcp_bpf_push                         //inet_release
      do_tcp_sendpages                    sock->ops->release
       sk_stream_wait_memory          	   // tcp_close
          sk_wait_event                      sk->sk_prot->close
           release_sock(__sk);
            ***

                                                lock_sock(sk);
                                                  __tcp_close
                                                    sock_orphan(sk)
                                                      sk->sk_wq  = NULL
                                                release_sock
            ****
           lock_sock(__sk);
          remove_wait_queue(sk_sleep(sk), &wait);
             sk_sleep(sk)
             //NULL pointer dereference
             &rcu_dereference_raw(sk->sk_wq)->wait

While waiting for memory in thread1, the socket is released with its wait
queue because thread2 has closed it. This caused by tcp_bpf_send_verdict
didn't increase the f_count of psock->sk_redir->sk_socket->file in thread1.

Avoid it by keeping a reference to the socket file while redirect sock wait
send memory. Refer to [1].

[1] https://lore.kernel.org/netdev/20190211090949.18560-1-jakub@cloudflare.com/

Signed-off-by: Liu Jian <liujian56@huawei.com>
---
 net/ipv4/tcp_bpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index a1626afe87a1..201375829367 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -125,9 +125,17 @@ static int tcp_bpf_push_locked(struct sock *sk, struct sk_msg *msg,
 {
 	int ret;
 
+	/* Hold on to socket wait queue. */
+	if (sk->sk_socket && sk->sk_socket->file)
+		get_file(sk->sk_socket->file);
+
 	lock_sock(sk);
 	ret = tcp_bpf_push(sk, msg, apply_bytes, flags, uncharge);
 	release_sock(sk);
+
+	if (sk->sk_socket && sk->sk_socket->file)
+		fput(sk->sk_socket->file);
+
 	return ret;
 }
 
-- 
2.17.1

RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[John Fastabend]

Liu Jian wrote:
> Fix the below NULL pointer dereference:
> 
> [   14.471200] Call Trace:
> [   14.471562]  <TASK>
> [   14.471882]  lock_acquire+0x245/0x2e0
> [   14.472416]  ? remove_wait_queue+0x12/0x50
> [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> [   14.474318]  ? remove_wait_queue+0x12/0x50
> [   14.474907]  remove_wait_queue+0x12/0x50
> [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> [   14.476704]  do_tcp_sendpages+0x287/0x600
> [   14.477283]  tcp_bpf_push+0xab/0x260
> [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> [   14.480311]  sock_sendmsg+0x2d/0x40
> [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> [   14.482048]  ___sys_sendmsg+0x78/0xb0
> [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> [   14.483215]  ? __do_fault+0x2a/0x1a0
> [   14.483738]  ? do_fault+0x15e/0x5d0
> [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> [   14.484874]  ? lock_is_held_type+0xdf/0x130
> [   14.485474]  ? find_held_lock+0x2d/0x90
> [   14.486046]  ? __sys_sendmsg+0x41/0x70
> [   14.486587]  __sys_sendmsg+0x41/0x70
> [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> [   14.487822]  do_syscall_64+0x34/0x80
> [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> The test scene as following flow:
> thread1                               thread2
> -----------                           ---------------
>  tcp_bpf_sendmsg
>   tcp_bpf_send_verdict
>    tcp_bpf_sendmsg_redir              sock_close
>     tcp_bpf_push_locked                 __sock_release
>      tcp_bpf_push                         //inet_release
>       do_tcp_sendpages                    sock->ops->release
>        sk_stream_wait_memory          	   // tcp_close
>           sk_wait_event                      sk->sk_prot->close
>            release_sock(__sk);
>             ***
> 
>                                                 lock_sock(sk);
>                                                   __tcp_close
>                                                     sock_orphan(sk)
>                                                       sk->sk_wq  = NULL
>                                                 release_sock
>             ****
>            lock_sock(__sk);
>           remove_wait_queue(sk_sleep(sk), &wait);
>              sk_sleep(sk)
>              //NULL pointer dereference
>              &rcu_dereference_raw(sk->sk_wq)->wait
> 
> While waiting for memory in thread1, the socket is released with its wait
> queue because thread2 has closed it. This caused by tcp_bpf_send_verdict
> didn't increase the f_count of psock->sk_redir->sk_socket->file in thread1.
> 
> Avoid it by keeping a reference to the socket file while redirect sock wait
> send memory. Refer to [1].
> 
> [1] https://lore.kernel.org/netdev/20190211090949.18560-1-jakub@cloudflare.com/
> 
> Signed-off-by: Liu Jian <liujian56@huawei.com>

Thanks for the detailed commit message its necessary to understand
the problem without spending hours deciphering it myself.

When I looked at [1] we solved a simliar problem by using
the MSG_DONTWAIT flag so that the error was pushed back to
the sending.

Can we do the same thing here? The nice bit here is the error
would get all the way back to the sending socket so userspace
could decide how to handle it? Did I miss something?

> ---
>  net/ipv4/tcp_bpf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index a1626afe87a1..201375829367 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -125,9 +125,17 @@ static int tcp_bpf_push_locked(struct sock *sk, struct sk_msg *msg,
>  {
>  	int ret;
>  
> +	/* Hold on to socket wait queue. */
> +	if (sk->sk_socket && sk->sk_socket->file)
> +		get_file(sk->sk_socket->file);
> +
>  	lock_sock(sk);
>  	ret = tcp_bpf_push(sk, msg, apply_bytes, flags, uncharge);
>  	release_sock(sk);
> +
> +	if (sk->sk_socket && sk->sk_socket->file)
> +		fput(sk->sk_socket->file);
> +
>  	return ret;
>  }
>  
> -- 
> 2.17.1
> 

RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[liujian (CE)]

> -----Original Message-----
> From: John Fastabend [mailto:john.fastabend@gmail.com]
> Sent: Wednesday, August 17, 2022 8:55 AM
> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> jakub@cloudflare.com; edumazet@google.com; davem@davemloft.net;
> yoshfuji@linux-ipv6.org; dsahern@kernel.org; kuba@kernel.org;
> pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com; ast@kernel.org;
> daniel@iogearbox.net; martin.lau@linux.dev; song@kernel.org; yhs@fb.com;
> kpsingh@kernel.org; sdf@google.com; haoluo@google.com;
> jolsa@kernel.org; shuah@kernel.org; bpf@vger.kernel.org
> Cc: liujian (CE) <liujian56@huawei.com>
> Subject: RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
> while wait_memory
> 
> Liu Jian wrote:
> > Fix the below NULL pointer dereference:
> >
> > [   14.471200] Call Trace:
> > [   14.471562]  <TASK>
> > [   14.471882]  lock_acquire+0x245/0x2e0
> > [   14.472416]  ? remove_wait_queue+0x12/0x50
> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> > [   14.474318]  ? remove_wait_queue+0x12/0x50
> > [   14.474907]  remove_wait_queue+0x12/0x50
> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> > [   14.476704]  do_tcp_sendpages+0x287/0x600
> > [   14.477283]  tcp_bpf_push+0xab/0x260
> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> > [   14.480311]  sock_sendmsg+0x2d/0x40
> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> > [   14.483215]  ? __do_fault+0x2a/0x1a0
> > [   14.483738]  ? do_fault+0x15e/0x5d0
> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
> > [   14.485474]  ? find_held_lock+0x2d/0x90
> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
> > [   14.486587]  __sys_sendmsg+0x41/0x70
> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> > [   14.487822]  do_syscall_64+0x34/0x80
> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
> > The test scene as following flow:
> > thread1                               thread2
> > -----------                           ---------------
> >  tcp_bpf_sendmsg
> >   tcp_bpf_send_verdict
> >    tcp_bpf_sendmsg_redir              sock_close
> >     tcp_bpf_push_locked                 __sock_release
> >      tcp_bpf_push                         //inet_release
> >       do_tcp_sendpages                    sock->ops->release
> >        sk_stream_wait_memory          	   // tcp_close
> >           sk_wait_event                      sk->sk_prot->close
> >            release_sock(__sk);
> >             ***
> >
> >                                                 lock_sock(sk);
> >                                                   __tcp_close
> >                                                     sock_orphan(sk)
> >                                                       sk->sk_wq  = NULL
> >                                                 release_sock
> >             ****
> >            lock_sock(__sk);
> >           remove_wait_queue(sk_sleep(sk), &wait);
> >              sk_sleep(sk)
> >              //NULL pointer dereference
> >              &rcu_dereference_raw(sk->sk_wq)->wait
> >
> > While waiting for memory in thread1, the socket is released with its
> > wait queue because thread2 has closed it. This caused by
> > tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
> >sk_socket->file in thread1.
> >
> > Avoid it by keeping a reference to the socket file while redirect sock
> > wait send memory. Refer to [1].
> >
> > [1]
> > https://lore.kernel.org/netdev/20190211090949.18560-1-jakub@cloudflare
> > .com/
> >
> > Signed-off-by: Liu Jian <liujian56@huawei.com>
> 
> Thanks for the detailed commit message its necessary to understand the
> problem without spending hours deciphering it myself.
> 
> When I looked at [1] we solved a simliar problem by using the
> MSG_DONTWAIT flag so that the error was pushed back to the sending.
> 
> Can we do the same thing here? The nice bit here is the error would get all
> the way back to the sending socket so userspace could decide how to handle
> it? Did I miss something?
> 
[1] works in sk_psock_backlog function, it can not be detected by the userspace app.
But here, the problem is that app wants this to be a blocked system call.
If the MSG_DONTWAIT flag is forcibly added, this changes the function behavior.

> > ---
> >  net/ipv4/tcp_bpf.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index
> > a1626afe87a1..201375829367 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -125,9 +125,17 @@ static int tcp_bpf_push_locked(struct sock *sk,
> > struct sk_msg *msg,  {
> >  	int ret;
> >
> > +	/* Hold on to socket wait queue. */
> > +	if (sk->sk_socket && sk->sk_socket->file)
> > +		get_file(sk->sk_socket->file);
> > +
> >  	lock_sock(sk);
> >  	ret = tcp_bpf_push(sk, msg, apply_bytes, flags, uncharge);
> >  	release_sock(sk);
> > +
> > +	if (sk->sk_socket && sk->sk_socket->file)
> > +		fput(sk->sk_socket->file);
> > +
> >  	return ret;
> >  }
> >
> > --
> > 2.17.1
> >
> 

RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[John Fastabend]

liujian (CE) wrote:
> 
> 
> > -----Original Message-----
> > From: John Fastabend [mailto:john.fastabend@gmail.com]
> > Sent: Wednesday, August 17, 2022 8:55 AM
> > To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> > jakub@cloudflare.com; edumazet@google.com; davem@davemloft.net;
> > yoshfuji@linux-ipv6.org; dsahern@kernel.org; kuba@kernel.org;
> > pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com; ast@kernel.org;
> > daniel@iogearbox.net; martin.lau@linux.dev; song@kernel.org; yhs@fb.com;
> > kpsingh@kernel.org; sdf@google.com; haoluo@google.com;
> > jolsa@kernel.org; shuah@kernel.org; bpf@vger.kernel.org
> > Cc: liujian (CE) <liujian56@huawei.com>
> > Subject: RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
> > while wait_memory
> > 
> > Liu Jian wrote:
> > > Fix the below NULL pointer dereference:
> > >
> > > [   14.471200] Call Trace:
> > > [   14.471562]  <TASK>
> > > [   14.471882]  lock_acquire+0x245/0x2e0
> > > [   14.472416]  ? remove_wait_queue+0x12/0x50
> > > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> > > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> > > [   14.474318]  ? remove_wait_queue+0x12/0x50
> > > [   14.474907]  remove_wait_queue+0x12/0x50
> > > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> > > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> > > [   14.476704]  do_tcp_sendpages+0x287/0x600
> > > [   14.477283]  tcp_bpf_push+0xab/0x260
> > > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> > > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> > > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> > > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> > > [   14.480311]  sock_sendmsg+0x2d/0x40
> > > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> > > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> > > [   14.482048]  ___sys_sendmsg+0x78/0xb0
> > > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> > > [   14.483215]  ? __do_fault+0x2a/0x1a0
> > > [   14.483738]  ? do_fault+0x15e/0x5d0
> > > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> > > [   14.484874]  ? lock_is_held_type+0xdf/0x130
> > > [   14.485474]  ? find_held_lock+0x2d/0x90
> > > [   14.486046]  ? __sys_sendmsg+0x41/0x70
> > > [   14.486587]  __sys_sendmsg+0x41/0x70
> > > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> > > [   14.487822]  do_syscall_64+0x34/0x80
> > > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > >
> > > The test scene as following flow:
> > > thread1                               thread2
> > > -----------                           ---------------
> > >  tcp_bpf_sendmsg
> > >   tcp_bpf_send_verdict
> > >    tcp_bpf_sendmsg_redir              sock_close
> > >     tcp_bpf_push_locked                 __sock_release
> > >      tcp_bpf_push                         //inet_release
> > >       do_tcp_sendpages                    sock->ops->release
> > >        sk_stream_wait_memory          	   // tcp_close
> > >           sk_wait_event                      sk->sk_prot->close
> > >            release_sock(__sk);
> > >             ***
> > >
> > >                                                 lock_sock(sk);
> > >                                                   __tcp_close
> > >                                                     sock_orphan(sk)
> > >                                                       sk->sk_wq  = NULL
> > >                                                 release_sock
> > >             ****
> > >            lock_sock(__sk);
> > >           remove_wait_queue(sk_sleep(sk), &wait);
> > >              sk_sleep(sk)
> > >              //NULL pointer dereference
> > >              &rcu_dereference_raw(sk->sk_wq)->wait
> > >
> > > While waiting for memory in thread1, the socket is released with its
> > > wait queue because thread2 has closed it. This caused by
> > > tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
> > >sk_socket->file in thread1.
> > >
> > > Avoid it by keeping a reference to the socket file while redirect sock
> > > wait send memory. Refer to [1].
> > >
> > > [1]
> > > https://lore.kernel.org/netdev/20190211090949.18560-1-jakub@cloudflare
> > > .com/
> > >
> > > Signed-off-by: Liu Jian <liujian56@huawei.com>
> > 
> > Thanks for the detailed commit message its necessary to understand the
> > problem without spending hours deciphering it myself.
> > 
> > When I looked at [1] we solved a simliar problem by using the
> > MSG_DONTWAIT flag so that the error was pushed back to the sending.
> > 
> > Can we do the same thing here? The nice bit here is the error would get all
> > the way back to the sending socket so userspace could decide how to handle
> > it? Did I miss something?
> > 
> [1] works in sk_psock_backlog function, it can not be detected by the userspace app.
> But here, the problem is that app wants this to be a blocked system call.
> If the MSG_DONTWAIT flag is forcibly added, this changes the function behavior.
> 

Ah right. We could push it to the sk_psock_backlog as another option similar
to what sk_psock_verdict_apply() does with sk_psock_skb_redirect(). The
problem would be we don't have an skb here and then instead of the
stream wait logic we would be using the backlog logic which might create
some subtle change. Seems a bit intrusive to me.

I don't have any better ideas off-hand even though reaching into the file
like above in the patch is not ideal.

Maybe Jakub has some thoughts?

Thanks!
John

Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[Jakub Sitnicki]

On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
> Fix the below NULL pointer dereference:
>
> [   14.471200] Call Trace:
> [   14.471562]  <TASK>
> [   14.471882]  lock_acquire+0x245/0x2e0
> [   14.472416]  ? remove_wait_queue+0x12/0x50
> [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> [   14.474318]  ? remove_wait_queue+0x12/0x50
> [   14.474907]  remove_wait_queue+0x12/0x50
> [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> [   14.476704]  do_tcp_sendpages+0x287/0x600
> [   14.477283]  tcp_bpf_push+0xab/0x260
> [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> [   14.480311]  sock_sendmsg+0x2d/0x40
> [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> [   14.482048]  ___sys_sendmsg+0x78/0xb0
> [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> [   14.483215]  ? __do_fault+0x2a/0x1a0
> [   14.483738]  ? do_fault+0x15e/0x5d0
> [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> [   14.484874]  ? lock_is_held_type+0xdf/0x130
> [   14.485474]  ? find_held_lock+0x2d/0x90
> [   14.486046]  ? __sys_sendmsg+0x41/0x70
> [   14.486587]  __sys_sendmsg+0x41/0x70
> [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> [   14.487822]  do_syscall_64+0x34/0x80
> [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> The test scene as following flow:
> thread1                               thread2
> -----------                           ---------------
>  tcp_bpf_sendmsg
>   tcp_bpf_send_verdict
>    tcp_bpf_sendmsg_redir              sock_close
>     tcp_bpf_push_locked                 __sock_release
>      tcp_bpf_push                         //inet_release
>       do_tcp_sendpages                    sock->ops->release
>        sk_stream_wait_memory          	   // tcp_close
>           sk_wait_event                      sk->sk_prot->close
>            release_sock(__sk);
>             ***
>
>                                                 lock_sock(sk);
>                                                   __tcp_close
>                                                     sock_orphan(sk)
>                                                       sk->sk_wq  = NULL
>                                                 release_sock
>             ****
>            lock_sock(__sk);
>           remove_wait_queue(sk_sleep(sk), &wait);
>              sk_sleep(sk)
>              //NULL pointer dereference
>              &rcu_dereference_raw(sk->sk_wq)->wait
>
> While waiting for memory in thread1, the socket is released with its wait
> queue because thread2 has closed it. This caused by tcp_bpf_send_verdict
> didn't increase the f_count of psock->sk_redir->sk_socket->file in thread1.

I'm not sure about this approach. Keeping a closed sock file alive, just
so we can wakeup from sleep, seems like wasted effort.

__tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN | SEND_SHUTDOWN. So we
will return from sk_stream_wait_memory via the do_error path.

SEND_SHUTDOWN might be set because socket got closed and orphaned - dead
and detached from its file, like in this case.

So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the wait queue.

[...]

RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[liujian (CE)]

> -----Original Message-----
> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
> Sent: Friday, August 19, 2022 4:39 PM
> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> edumazet@google.com
> Cc: davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com;
> ast@kernel.org; daniel@iogearbox.net; martin.lau@linux.dev;
> song@kernel.org; yhs@fb.com; kpsingh@kernel.org; sdf@google.com;
> haoluo@google.com; jolsa@kernel.org; shuah@kernel.org;
> bpf@vger.kernel.org
> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
> while wait_memory
> 
> On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
> > Fix the below NULL pointer dereference:
> >
> > [   14.471200] Call Trace:
> > [   14.471562]  <TASK>
> > [   14.471882]  lock_acquire+0x245/0x2e0
> > [   14.472416]  ? remove_wait_queue+0x12/0x50
> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> > [   14.474318]  ? remove_wait_queue+0x12/0x50
> > [   14.474907]  remove_wait_queue+0x12/0x50
> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> > [   14.476704]  do_tcp_sendpages+0x287/0x600
> > [   14.477283]  tcp_bpf_push+0xab/0x260
> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> > [   14.480311]  sock_sendmsg+0x2d/0x40
> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> > [   14.483215]  ? __do_fault+0x2a/0x1a0
> > [   14.483738]  ? do_fault+0x15e/0x5d0
> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
> > [   14.485474]  ? find_held_lock+0x2d/0x90
> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
> > [   14.486587]  __sys_sendmsg+0x41/0x70
> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> > [   14.487822]  do_syscall_64+0x34/0x80
> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
> > The test scene as following flow:
> > thread1                               thread2
> > -----------                           ---------------
> >  tcp_bpf_sendmsg
> >   tcp_bpf_send_verdict
> >    tcp_bpf_sendmsg_redir              sock_close
> >     tcp_bpf_push_locked                 __sock_release
> >      tcp_bpf_push                         //inet_release
> >       do_tcp_sendpages                    sock->ops->release
> >        sk_stream_wait_memory          	   // tcp_close
> >           sk_wait_event                      sk->sk_prot->close
> >            release_sock(__sk);
> >             ***
> >
> >                                                 lock_sock(sk);
> >                                                   __tcp_close
> >                                                     sock_orphan(sk)
> >                                                       sk->sk_wq  = NULL
> >                                                 release_sock
> >             ****
> >            lock_sock(__sk);
> >           remove_wait_queue(sk_sleep(sk), &wait);
> >              sk_sleep(sk)
> >              //NULL pointer dereference
> >              &rcu_dereference_raw(sk->sk_wq)->wait
> >
> > While waiting for memory in thread1, the socket is released with its
> > wait queue because thread2 has closed it. This caused by
> > tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
> >sk_socket->file in thread1.
> 
> I'm not sure about this approach. Keeping a closed sock file alive, just so we
> can wakeup from sleep, seems like wasted effort.
> 
> __tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN | SEND_SHUTDOWN.
> So we will return from sk_stream_wait_memory via the do_error path.
> 
> SEND_SHUTDOWN might be set because socket got closed and orphaned -
> dead and detached from its file, like in this case.
> 
> So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
> SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the wait
> queue.
> 
> [...]
As jakub's approach, this problem can be solved.

diff --git a/include/net/sock.h b/include/net/sock.h
index a7273b289188..a3dab7140f1e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1998,6 +1998,8 @@ static inline void sk_set_socket(struct sock *sk, struct socket *sock)
 static inline wait_queue_head_t *sk_sleep(struct sock *sk)
 {
        BUILD_BUG_ON(offsetof(struct socket_wq, wait) != 0);
+       if (sock_flag(sk, SOCK_DEAD))
+               return NULL;
        return &rcu_dereference_raw(sk->sk_wq)->wait;
 }
 /* Detach socket from process context.
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 9860bb9a847c..da1be17d0b19 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -51,6 +51,8 @@ void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry
 {
        unsigned long flags;
 
+       if (wq_head == NULL)
+               return;
        spin_lock_irqsave(&wq_head->lock, flags);
        __remove_wait_queue(wq_head, wq_entry);
        spin_unlock_irqrestore(&wq_head->lock, flags);

Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[Jakub Sitnicki]

On Fri, Aug 19, 2022 at 10:01 AM GMT, liujian (CE) wrote:
>> -----Original Message-----
>> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
>> Sent: Friday, August 19, 2022 4:39 PM
>> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
>> edumazet@google.com
>> Cc: davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
>> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com;
>> ast@kernel.org; daniel@iogearbox.net; martin.lau@linux.dev;
>> song@kernel.org; yhs@fb.com; kpsingh@kernel.org; sdf@google.com;
>> haoluo@google.com; jolsa@kernel.org; shuah@kernel.org;
>> bpf@vger.kernel.org
>> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
>> while wait_memory
>> 
>> On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
>> > Fix the below NULL pointer dereference:
>> >
>> > [   14.471200] Call Trace:
>> > [   14.471562]  <TASK>
>> > [   14.471882]  lock_acquire+0x245/0x2e0
>> > [   14.472416]  ? remove_wait_queue+0x12/0x50
>> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
>> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
>> > [   14.474318]  ? remove_wait_queue+0x12/0x50
>> > [   14.474907]  remove_wait_queue+0x12/0x50
>> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
>> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
>> > [   14.476704]  do_tcp_sendpages+0x287/0x600
>> > [   14.477283]  tcp_bpf_push+0xab/0x260
>> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
>> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
>> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
>> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
>> > [   14.480311]  sock_sendmsg+0x2d/0x40
>> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
>> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
>> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
>> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
>> > [   14.483215]  ? __do_fault+0x2a/0x1a0
>> > [   14.483738]  ? do_fault+0x15e/0x5d0
>> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
>> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
>> > [   14.485474]  ? find_held_lock+0x2d/0x90
>> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
>> > [   14.486587]  __sys_sendmsg+0x41/0x70
>> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
>> > [   14.487822]  do_syscall_64+0x34/0x80
>> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> >
>> > The test scene as following flow:
>> > thread1                               thread2
>> > -----------                           ---------------
>> >  tcp_bpf_sendmsg
>> >   tcp_bpf_send_verdict
>> >    tcp_bpf_sendmsg_redir              sock_close
>> >     tcp_bpf_push_locked                 __sock_release
>> >      tcp_bpf_push                         //inet_release
>> >       do_tcp_sendpages                    sock->ops->release
>> >        sk_stream_wait_memory          	   // tcp_close
>> >           sk_wait_event                      sk->sk_prot->close
>> >            release_sock(__sk);
>> >             ***
>> >
>> >                                                 lock_sock(sk);
>> >                                                   __tcp_close
>> >                                                     sock_orphan(sk)
>> >                                                       sk->sk_wq  = NULL
>> >                                                 release_sock
>> >             ****
>> >            lock_sock(__sk);
>> >           remove_wait_queue(sk_sleep(sk), &wait);
>> >              sk_sleep(sk)
>> >              //NULL pointer dereference
>> >              &rcu_dereference_raw(sk->sk_wq)->wait
>> >
>> > While waiting for memory in thread1, the socket is released with its
>> > wait queue because thread2 has closed it. This caused by
>> > tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
>> >sk_socket->file in thread1.
>> 
>> I'm not sure about this approach. Keeping a closed sock file alive, just so we
>> can wakeup from sleep, seems like wasted effort.
>> 
>> __tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN | SEND_SHUTDOWN.
>> So we will return from sk_stream_wait_memory via the do_error path.
>> 
>> SEND_SHUTDOWN might be set because socket got closed and orphaned -
>> dead and detached from its file, like in this case.
>> 
>> So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
>> SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the wait
>> queue.
>> 
>> [...]
> As jakub's approach, this problem can be solved.
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index a7273b289188..a3dab7140f1e 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1998,6 +1998,8 @@ static inline void sk_set_socket(struct sock *sk, struct socket *sock)
>  static inline wait_queue_head_t *sk_sleep(struct sock *sk)
>  {
>         BUILD_BUG_ON(offsetof(struct socket_wq, wait) != 0);
> +       if (sock_flag(sk, SOCK_DEAD))
> +               return NULL;
>         return &rcu_dereference_raw(sk->sk_wq)->wait;
>  }
>  /* Detach socket from process context.
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index 9860bb9a847c..da1be17d0b19 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -51,6 +51,8 @@ void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry
>  {
>         unsigned long flags;
>  
> +       if (wq_head == NULL)
> +               return;
>         spin_lock_irqsave(&wq_head->lock, flags);
>         __remove_wait_queue(wq_head, wq_entry);
>         spin_unlock_irqrestore(&wq_head->lock, flags);

I don't know if we want to change the contract for sk_sleep()
remove_wait_queue() so that they accept dead sockets or nulls.

How about just:

diff --git a/net/core/stream.c b/net/core/stream.c
index ccc083cdef23..1105057ce00a 100644
--- a/net/core/stream.c
+++ b/net/core/stream.c
@@ -159,7 +159,8 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p)
                *timeo_p = current_timeo;
        }
 out:
-       remove_wait_queue(sk_sleep(sk), &wait);
+       if (!sock_flag(sk, SOCK_DEAD))
+               remove_wait_queue(sk_sleep(sk), &wait);
        return err;

 do_error:

RE: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[liujian (CE)]

> -----Original Message-----
> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
> Sent: Friday, August 19, 2022 6:35 PM
> To: liujian (CE) <liujian56@huawei.com>
> Cc: john.fastabend@gmail.com; edumazet@google.com;
> davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com;
> ast@kernel.org; daniel@iogearbox.net; martin.lau@linux.dev;
> song@kernel.org; yhs@fb.com; kpsingh@kernel.org; sdf@google.com;
> haoluo@google.com; jolsa@kernel.org; shuah@kernel.org;
> bpf@vger.kernel.org
> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
> while wait_memory
> 
> 
> On Fri, Aug 19, 2022 at 10:01 AM GMT, liujian (CE) wrote:
> >> -----Original Message-----
> >> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
> >> Sent: Friday, August 19, 2022 4:39 PM
> >> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> >> edumazet@google.com
> >> Cc: davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
> >> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org;
> >> mykolal@fb.com; ast@kernel.org; daniel@iogearbox.net;
> >> martin.lau@linux.dev; song@kernel.org; yhs@fb.com;
> >> kpsingh@kernel.org; sdf@google.com; haoluo@google.com;
> >> jolsa@kernel.org; shuah@kernel.org; bpf@vger.kernel.org
> >> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket
> >> file while wait_memory
> >>
> >> On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
> >> > Fix the below NULL pointer dereference:
> >> >
> >> > [   14.471200] Call Trace:
> >> > [   14.471562]  <TASK>
> >> > [   14.471882]  lock_acquire+0x245/0x2e0
> >> > [   14.472416]  ? remove_wait_queue+0x12/0x50
> >> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> >> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> >> > [   14.474318]  ? remove_wait_queue+0x12/0x50
> >> > [   14.474907]  remove_wait_queue+0x12/0x50
> >> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> >> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> >> > [   14.476704]  do_tcp_sendpages+0x287/0x600
> >> > [   14.477283]  tcp_bpf_push+0xab/0x260
> >> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> >> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> >> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> >> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> >> > [   14.480311]  sock_sendmsg+0x2d/0x40
> >> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> >> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> >> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
> >> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> >> > [   14.483215]  ? __do_fault+0x2a/0x1a0
> >> > [   14.483738]  ? do_fault+0x15e/0x5d0
> >> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> >> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
> >> > [   14.485474]  ? find_held_lock+0x2d/0x90
> >> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
> >> > [   14.486587]  __sys_sendmsg+0x41/0x70
> >> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> >> > [   14.487822]  do_syscall_64+0x34/0x80
> >> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >> >
> >> > The test scene as following flow:
> >> > thread1                               thread2
> >> > -----------                           ---------------
> >> >  tcp_bpf_sendmsg
> >> >   tcp_bpf_send_verdict
> >> >    tcp_bpf_sendmsg_redir              sock_close
> >> >     tcp_bpf_push_locked                 __sock_release
> >> >      tcp_bpf_push                         //inet_release
> >> >       do_tcp_sendpages                    sock->ops->release
> >> >        sk_stream_wait_memory          	   // tcp_close
> >> >           sk_wait_event                      sk->sk_prot->close
> >> >            release_sock(__sk);
> >> >             ***
> >> >
> >> >                                                 lock_sock(sk);
> >> >                                                   __tcp_close
> >> >                                                     sock_orphan(sk)
> >> >                                                       sk->sk_wq  = NULL
> >> >                                                 release_sock
> >> >             ****
> >> >            lock_sock(__sk);
> >> >           remove_wait_queue(sk_sleep(sk), &wait);
> >> >              sk_sleep(sk)
> >> >              //NULL pointer dereference
> >> >              &rcu_dereference_raw(sk->sk_wq)->wait
> >> >
> >> > While waiting for memory in thread1, the socket is released with
> >> >its  wait queue because thread2 has closed it. This caused by
> >> >tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
> >> >sk_socket->file in thread1.
> >>
> >> I'm not sure about this approach. Keeping a closed sock file alive,
> >> just so we can wakeup from sleep, seems like wasted effort.
> >>
> >> __tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN |
> SEND_SHUTDOWN.
> >> So we will return from sk_stream_wait_memory via the do_error path.
> >>
> >> SEND_SHUTDOWN might be set because socket got closed and orphaned
> -
> >> dead and detached from its file, like in this case.
> >>
> >> So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
> >> SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the
> wait
> >> queue.
> >>
> >> [...]
> > As jakub's approach, this problem can be solved.
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h index
> > a7273b289188..a3dab7140f1e 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1998,6 +1998,8 @@ static inline void sk_set_socket(struct sock
> > *sk, struct socket *sock)  static inline wait_queue_head_t
> > *sk_sleep(struct sock *sk)  {
> >         BUILD_BUG_ON(offsetof(struct socket_wq, wait) != 0);
> > +       if (sock_flag(sk, SOCK_DEAD))
> > +               return NULL;
> >         return &rcu_dereference_raw(sk->sk_wq)->wait;
> >  }
> >  /* Detach socket from process context.
> > diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c index
> > 9860bb9a847c..da1be17d0b19 100644
> > --- a/kernel/sched/wait.c
> > +++ b/kernel/sched/wait.c
> > @@ -51,6 +51,8 @@ void remove_wait_queue(struct wait_queue_head
> > *wq_head, struct wait_queue_entry  {
> >         unsigned long flags;
> >
> > +       if (wq_head == NULL)
> > +               return;
> >         spin_lock_irqsave(&wq_head->lock, flags);
> >         __remove_wait_queue(wq_head, wq_entry);
> >         spin_unlock_irqrestore(&wq_head->lock, flags);
> 
> I don't know if we want to change the contract for sk_sleep()
> remove_wait_queue() so that they accept dead sockets or nulls.
> 
> How about just:

It is all ok to me, thank you. Cloud you provide a format patch?

Tested-by: Liu Jian <liujian56@huawei.com>
> 
> diff --git a/net/core/stream.c b/net/core/stream.c index
> ccc083cdef23..1105057ce00a 100644
> --- a/net/core/stream.c
> +++ b/net/core/stream.c
> @@ -159,7 +159,8 @@ int sk_stream_wait_memory(struct sock *sk, long
> *timeo_p)
>                 *timeo_p = current_timeo;
>         }
>  out:
> -       remove_wait_queue(sk_sleep(sk), &wait);
> +       if (!sock_flag(sk, SOCK_DEAD))
> +               remove_wait_queue(sk_sleep(sk), &wait);
>         return err;
> 
>  do_error:
> 
> 
> 

Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[Jakub Sitnicki]

On Sat, Aug 20, 2022 at 03:01 AM GMT, liujian (CE) wrote:
>> -----Original Message-----
>> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
>> Sent: Friday, August 19, 2022 6:35 PM
>> To: liujian (CE) <liujian56@huawei.com>
>> Cc: john.fastabend@gmail.com; edumazet@google.com;
>> davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
>> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com;
>> ast@kernel.org; daniel@iogearbox.net; martin.lau@linux.dev;
>> song@kernel.org; yhs@fb.com; kpsingh@kernel.org; sdf@google.com;
>> haoluo@google.com; jolsa@kernel.org; shuah@kernel.org;
>> bpf@vger.kernel.org
>> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
>> while wait_memory
>> 
>> 
>> On Fri, Aug 19, 2022 at 10:01 AM GMT, liujian (CE) wrote:
>> >> -----Original Message-----
>> >> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
>> >> Sent: Friday, August 19, 2022 4:39 PM
>> >> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
>> >> edumazet@google.com
>> >> Cc: davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
>> >> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org;
>> >> mykolal@fb.com; ast@kernel.org; daniel@iogearbox.net;
>> >> martin.lau@linux.dev; song@kernel.org; yhs@fb.com;
>> >> kpsingh@kernel.org; sdf@google.com; haoluo@google.com;
>> >> jolsa@kernel.org; shuah@kernel.org; bpf@vger.kernel.org
>> >> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket
>> >> file while wait_memory
>> >>
>> >> On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
>> >> > Fix the below NULL pointer dereference:
>> >> >
>> >> > [   14.471200] Call Trace:
>> >> > [   14.471562]  <TASK>
>> >> > [   14.471882]  lock_acquire+0x245/0x2e0
>> >> > [   14.472416]  ? remove_wait_queue+0x12/0x50
>> >> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
>> >> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
>> >> > [   14.474318]  ? remove_wait_queue+0x12/0x50
>> >> > [   14.474907]  remove_wait_queue+0x12/0x50
>> >> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
>> >> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
>> >> > [   14.476704]  do_tcp_sendpages+0x287/0x600
>> >> > [   14.477283]  tcp_bpf_push+0xab/0x260
>> >> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
>> >> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
>> >> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
>> >> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
>> >> > [   14.480311]  sock_sendmsg+0x2d/0x40
>> >> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
>> >> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
>> >> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
>> >> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
>> >> > [   14.483215]  ? __do_fault+0x2a/0x1a0
>> >> > [   14.483738]  ? do_fault+0x15e/0x5d0
>> >> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
>> >> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
>> >> > [   14.485474]  ? find_held_lock+0x2d/0x90
>> >> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
>> >> > [   14.486587]  __sys_sendmsg+0x41/0x70
>> >> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
>> >> > [   14.487822]  do_syscall_64+0x34/0x80
>> >> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> >> >
>> >> > The test scene as following flow:
>> >> > thread1                               thread2
>> >> > -----------                           ---------------
>> >> >  tcp_bpf_sendmsg
>> >> >   tcp_bpf_send_verdict
>> >> >    tcp_bpf_sendmsg_redir              sock_close
>> >> >     tcp_bpf_push_locked                 __sock_release
>> >> >      tcp_bpf_push                         //inet_release
>> >> >       do_tcp_sendpages                    sock->ops->release
>> >> >        sk_stream_wait_memory          	   // tcp_close
>> >> >           sk_wait_event                      sk->sk_prot->close
>> >> >            release_sock(__sk);
>> >> >             ***
>> >> >
>> >> >                                                 lock_sock(sk);
>> >> >                                                   __tcp_close
>> >> >                                                     sock_orphan(sk)
>> >> >                                                       sk->sk_wq  = NULL
>> >> >                                                 release_sock
>> >> >             ****
>> >> >            lock_sock(__sk);
>> >> >           remove_wait_queue(sk_sleep(sk), &wait);
>> >> >              sk_sleep(sk)
>> >> >              //NULL pointer dereference
>> >> >              &rcu_dereference_raw(sk->sk_wq)->wait
>> >> >
>> >> > While waiting for memory in thread1, the socket is released with
>> >> >its  wait queue because thread2 has closed it. This caused by
>> >> >tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
>> >> >sk_socket->file in thread1.
>> >>
>> >> I'm not sure about this approach. Keeping a closed sock file alive,
>> >> just so we can wakeup from sleep, seems like wasted effort.
>> >>
>> >> __tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN |
>> SEND_SHUTDOWN.
>> >> So we will return from sk_stream_wait_memory via the do_error path.
>> >>
>> >> SEND_SHUTDOWN might be set because socket got closed and orphaned
>> -
>> >> dead and detached from its file, like in this case.
>> >>
>> >> So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
>> >> SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the
>> wait
>> >> queue.
>> >>
>> >> [...]
>> > As jakub's approach, this problem can be solved.
>> >
>> > diff --git a/include/net/sock.h b/include/net/sock.h index
>> > a7273b289188..a3dab7140f1e 100644
>> > --- a/include/net/sock.h
>> > +++ b/include/net/sock.h
>> > @@ -1998,6 +1998,8 @@ static inline void sk_set_socket(struct sock
>> > *sk, struct socket *sock)  static inline wait_queue_head_t
>> > *sk_sleep(struct sock *sk)  {
>> >         BUILD_BUG_ON(offsetof(struct socket_wq, wait) != 0);
>> > +       if (sock_flag(sk, SOCK_DEAD))
>> > +               return NULL;
>> >         return &rcu_dereference_raw(sk->sk_wq)->wait;
>> >  }
>> >  /* Detach socket from process context.
>> > diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c index
>> > 9860bb9a847c..da1be17d0b19 100644
>> > --- a/kernel/sched/wait.c
>> > +++ b/kernel/sched/wait.c
>> > @@ -51,6 +51,8 @@ void remove_wait_queue(struct wait_queue_head
>> > *wq_head, struct wait_queue_entry  {
>> >         unsigned long flags;
>> >
>> > +       if (wq_head == NULL)
>> > +               return;
>> >         spin_lock_irqsave(&wq_head->lock, flags);
>> >         __remove_wait_queue(wq_head, wq_entry);
>> >         spin_unlock_irqrestore(&wq_head->lock, flags);
>> 
>> I don't know if we want to change the contract for sk_sleep()
>> remove_wait_queue() so that they accept dead sockets or nulls.
>> 
>> How about just:
>
> It is all ok to me, thank you. Cloud you provide a format patch?
>
> Tested-by: Liu Jian <liujian56@huawei.com>

Feel free to pull it into your patch set. I'm a bit backlogged
ATM. Besides, we also want the selftest that you have added.

You can add Suggested-by if you want.

[...]

Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[Eric Dumazet]

On Fri, Aug 19, 2022 at 3:37 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
>
> On Fri, Aug 19, 2022 at 10:01 AM GMT, liujian (CE) wrote:
> >> -----Original Message-----
> >> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
> >> Sent: Friday, August 19, 2022 4:39 PM
> >> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
> >> edumazet@google.com
> >> Cc: davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
> >> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com;
> >> ast@kernel.org; daniel@iogearbox.net; martin.lau@linux.dev;
> >> song@kernel.org; yhs@fb.com; kpsingh@kernel.org; sdf@google.com;
> >> haoluo@google.com; jolsa@kernel.org; shuah@kernel.org;
> >> bpf@vger.kernel.org
> >> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
> >> while wait_memory
> >>
> >> On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
> >> > Fix the below NULL pointer dereference:
> >> >
> >> > [   14.471200] Call Trace:
> >> > [   14.471562]  <TASK>
> >> > [   14.471882]  lock_acquire+0x245/0x2e0
> >> > [   14.472416]  ? remove_wait_queue+0x12/0x50
> >> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
> >> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
> >> > [   14.474318]  ? remove_wait_queue+0x12/0x50
> >> > [   14.474907]  remove_wait_queue+0x12/0x50
> >> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
> >> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
> >> > [   14.476704]  do_tcp_sendpages+0x287/0x600
> >> > [   14.477283]  tcp_bpf_push+0xab/0x260
> >> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
> >> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
> >> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
> >> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
> >> > [   14.480311]  sock_sendmsg+0x2d/0x40
> >> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
> >> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
> >> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
> >> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
> >> > [   14.483215]  ? __do_fault+0x2a/0x1a0
> >> > [   14.483738]  ? do_fault+0x15e/0x5d0
> >> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
> >> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
> >> > [   14.485474]  ? find_held_lock+0x2d/0x90
> >> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
> >> > [   14.486587]  __sys_sendmsg+0x41/0x70
> >> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
> >> > [   14.487822]  do_syscall_64+0x34/0x80
> >> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >> >
> >> > The test scene as following flow:
> >> > thread1                               thread2
> >> > -----------                           ---------------
> >> >  tcp_bpf_sendmsg
> >> >   tcp_bpf_send_verdict
> >> >    tcp_bpf_sendmsg_redir              sock_close
> >> >     tcp_bpf_push_locked                 __sock_release
> >> >      tcp_bpf_push                         //inet_release
> >> >       do_tcp_sendpages                    sock->ops->release
> >> >        sk_stream_wait_memory                  // tcp_close
> >> >           sk_wait_event                      sk->sk_prot->close
> >> >            release_sock(__sk);
> >> >             ***
> >> >
> >> >                                                 lock_sock(sk);
> >> >                                                   __tcp_close
> >> >                                                     sock_orphan(sk)
> >> >                                                       sk->sk_wq  = NULL
> >> >                                                 release_sock
> >> >             ****
> >> >            lock_sock(__sk);
> >> >           remove_wait_queue(sk_sleep(sk), &wait);
> >> >              sk_sleep(sk)
> >> >              //NULL pointer dereference
> >> >              &rcu_dereference_raw(sk->sk_wq)->wait
> >> >
> >> > While waiting for memory in thread1, the socket is released with its
> >> > wait queue because thread2 has closed it. This caused by
> >> > tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
> >> >sk_socket->file in thread1.
> >>
> >> I'm not sure about this approach. Keeping a closed sock file alive, just so we
> >> can wakeup from sleep, seems like wasted effort.
> >>
> >> __tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN | SEND_SHUTDOWN.
> >> So we will return from sk_stream_wait_memory via the do_error path.
> >>
> >> SEND_SHUTDOWN might be set because socket got closed and orphaned -
> >> dead and detached from its file, like in this case.
> >>
> >> So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
> >> SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the wait
> >> queue.
> >>
> >> [...]
> > As jakub's approach, this problem can be solved.
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index a7273b289188..a3dab7140f1e 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1998,6 +1998,8 @@ static inline void sk_set_socket(struct sock *sk, struct socket *sock)
> >  static inline wait_queue_head_t *sk_sleep(struct sock *sk)
> >  {
> >         BUILD_BUG_ON(offsetof(struct socket_wq, wait) != 0);
> > +       if (sock_flag(sk, SOCK_DEAD))
> > +               return NULL;
> >         return &rcu_dereference_raw(sk->sk_wq)->wait;
> >  }
> >  /* Detach socket from process context.
> > diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> > index 9860bb9a847c..da1be17d0b19 100644
> > --- a/kernel/sched/wait.c
> > +++ b/kernel/sched/wait.c
> > @@ -51,6 +51,8 @@ void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry
> >  {
> >         unsigned long flags;
> >
> > +       if (wq_head == NULL)
> > +               return;
> >         spin_lock_irqsave(&wq_head->lock, flags);
> >         __remove_wait_queue(wq_head, wq_entry);
> >         spin_unlock_irqrestore(&wq_head->lock, flags);
>
> I don't know if we want to change the contract for sk_sleep()
> remove_wait_queue() so that they accept dead sockets or nulls.
>
> How about just:
>
> diff --git a/net/core/stream.c b/net/core/stream.c
> index ccc083cdef23..1105057ce00a 100644
> --- a/net/core/stream.c
> +++ b/net/core/stream.c
> @@ -159,7 +159,8 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p)
>                 *timeo_p = current_timeo;
>         }
>  out:
> -       remove_wait_queue(sk_sleep(sk), &wait);
> +       if (!sock_flag(sk, SOCK_DEAD))
> +               remove_wait_queue(sk_sleep(sk), &wait);
>         return err;
>
>  do_error:
>

OK, but what about tcp_msg_wait_data() and udp_msg_wait_data () ?
It seems they could be vulnerable as well..

Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file while wait_memory

[Jakub Sitnicki]

On Mon, Aug 22, 2022 at 08:52 AM -07, Eric Dumazet wrote:
> On Fri, Aug 19, 2022 at 3:37 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>>
>> On Fri, Aug 19, 2022 at 10:01 AM GMT, liujian (CE) wrote:
>> >> -----Original Message-----
>> >> From: Jakub Sitnicki [mailto:jakub@cloudflare.com]
>> >> Sent: Friday, August 19, 2022 4:39 PM
>> >> To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com;
>> >> edumazet@google.com
>> >> Cc: davem@davemloft.net; yoshfuji@linux-ipv6.org; dsahern@kernel.org;
>> >> kuba@kernel.org; pabeni@redhat.com; andrii@kernel.org; mykolal@fb.com;
>> >> ast@kernel.org; daniel@iogearbox.net; martin.lau@linux.dev;
>> >> song@kernel.org; yhs@fb.com; kpsingh@kernel.org; sdf@google.com;
>> >> haoluo@google.com; jolsa@kernel.org; shuah@kernel.org;
>> >> bpf@vger.kernel.org
>> >> Subject: Re: [PATCH bpf-next 1/2] sk_msg: Keep reference on socket file
>> >> while wait_memory
>> >>
>> >> On Mon, Aug 15, 2022 at 10:33 AM +08, Liu Jian wrote:
>> >> > Fix the below NULL pointer dereference:
>> >> >
>> >> > [   14.471200] Call Trace:
>> >> > [   14.471562]  <TASK>
>> >> > [   14.471882]  lock_acquire+0x245/0x2e0
>> >> > [   14.472416]  ? remove_wait_queue+0x12/0x50
>> >> > [   14.473014]  ? _raw_spin_lock_irqsave+0x17/0x50
>> >> > [   14.473681]  _raw_spin_lock_irqsave+0x3d/0x50
>> >> > [   14.474318]  ? remove_wait_queue+0x12/0x50
>> >> > [   14.474907]  remove_wait_queue+0x12/0x50
>> >> > [   14.475480]  sk_stream_wait_memory+0x20d/0x340
>> >> > [   14.476127]  ? do_wait_intr_irq+0x80/0x80
>> >> > [   14.476704]  do_tcp_sendpages+0x287/0x600
>> >> > [   14.477283]  tcp_bpf_push+0xab/0x260
>> >> > [   14.477817]  tcp_bpf_sendmsg_redir+0x297/0x500
>> >> > [   14.478461]  ? __local_bh_enable_ip+0x77/0xe0
>> >> > [   14.479096]  tcp_bpf_send_verdict+0x105/0x470
>> >> > [   14.479729]  tcp_bpf_sendmsg+0x318/0x4f0
>> >> > [   14.480311]  sock_sendmsg+0x2d/0x40
>> >> > [   14.480822]  ____sys_sendmsg+0x1b4/0x1c0
>> >> > [   14.481390]  ? copy_msghdr_from_user+0x62/0x80
>> >> > [   14.482048]  ___sys_sendmsg+0x78/0xb0
>> >> > [   14.482580]  ? vmf_insert_pfn_prot+0x91/0x150
>> >> > [   14.483215]  ? __do_fault+0x2a/0x1a0
>> >> > [   14.483738]  ? do_fault+0x15e/0x5d0
>> >> > [   14.484246]  ? __handle_mm_fault+0x56b/0x1040
>> >> > [   14.484874]  ? lock_is_held_type+0xdf/0x130
>> >> > [   14.485474]  ? find_held_lock+0x2d/0x90
>> >> > [   14.486046]  ? __sys_sendmsg+0x41/0x70
>> >> > [   14.486587]  __sys_sendmsg+0x41/0x70
>> >> > [   14.487105]  ? intel_pmu_drain_pebs_core+0x350/0x350
>> >> > [   14.487822]  do_syscall_64+0x34/0x80
>> >> > [   14.488345]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> >> >
>> >> > The test scene as following flow:
>> >> > thread1                               thread2
>> >> > -----------                           ---------------
>> >> >  tcp_bpf_sendmsg
>> >> >   tcp_bpf_send_verdict
>> >> >    tcp_bpf_sendmsg_redir              sock_close
>> >> >     tcp_bpf_push_locked                 __sock_release
>> >> >      tcp_bpf_push                         //inet_release
>> >> >       do_tcp_sendpages                    sock->ops->release
>> >> >        sk_stream_wait_memory                  // tcp_close
>> >> >           sk_wait_event                      sk->sk_prot->close
>> >> >            release_sock(__sk);
>> >> >             ***
>> >> >
>> >> >                                                 lock_sock(sk);
>> >> >                                                   __tcp_close
>> >> >                                                     sock_orphan(sk)
>> >> >                                                       sk->sk_wq  = NULL
>> >> >                                                 release_sock
>> >> >             ****
>> >> >            lock_sock(__sk);
>> >> >           remove_wait_queue(sk_sleep(sk), &wait);
>> >> >              sk_sleep(sk)
>> >> >              //NULL pointer dereference
>> >> >              &rcu_dereference_raw(sk->sk_wq)->wait
>> >> >
>> >> > While waiting for memory in thread1, the socket is released with its
>> >> > wait queue because thread2 has closed it. This caused by
>> >> > tcp_bpf_send_verdict didn't increase the f_count of psock->sk_redir-
>> >> >sk_socket->file in thread1.
>> >>
>> >> I'm not sure about this approach. Keeping a closed sock file alive, just so we
>> >> can wakeup from sleep, seems like wasted effort.
>> >>
>> >> __tcp_close sets sk->sk_shutdown = RCV_SHUTDOWN | SEND_SHUTDOWN.
>> >> So we will return from sk_stream_wait_memory via the do_error path.
>> >>
>> >> SEND_SHUTDOWN might be set because socket got closed and orphaned -
>> >> dead and detached from its file, like in this case.
>> >>
>> >> So, IMHO, we should check if SOCK_DEAD flag is set on wakeup due to
>> >> SEND_SHUTDOWN in sk_stream_wait_memory, before accessing the wait
>> >> queue.
>> >>
>> >> [...]
>> > As jakub's approach, this problem can be solved.
>> >
>> > diff --git a/include/net/sock.h b/include/net/sock.h
>> > index a7273b289188..a3dab7140f1e 100644
>> > --- a/include/net/sock.h
>> > +++ b/include/net/sock.h
>> > @@ -1998,6 +1998,8 @@ static inline void sk_set_socket(struct sock *sk, struct socket *sock)
>> >  static inline wait_queue_head_t *sk_sleep(struct sock *sk)
>> >  {
>> >         BUILD_BUG_ON(offsetof(struct socket_wq, wait) != 0);
>> > +       if (sock_flag(sk, SOCK_DEAD))
>> > +               return NULL;
>> >         return &rcu_dereference_raw(sk->sk_wq)->wait;
>> >  }
>> >  /* Detach socket from process context.
>> > diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
>> > index 9860bb9a847c..da1be17d0b19 100644
>> > --- a/kernel/sched/wait.c
>> > +++ b/kernel/sched/wait.c
>> > @@ -51,6 +51,8 @@ void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry
>> >  {
>> >         unsigned long flags;
>> >
>> > +       if (wq_head == NULL)
>> > +               return;
>> >         spin_lock_irqsave(&wq_head->lock, flags);
>> >         __remove_wait_queue(wq_head, wq_entry);
>> >         spin_unlock_irqrestore(&wq_head->lock, flags);
>>
>> I don't know if we want to change the contract for sk_sleep()
>> remove_wait_queue() so that they accept dead sockets or nulls.
>>
>> How about just:
>>
>> diff --git a/net/core/stream.c b/net/core/stream.c
>> index ccc083cdef23..1105057ce00a 100644
>> --- a/net/core/stream.c
>> +++ b/net/core/stream.c
>> @@ -159,7 +159,8 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p)
>>                 *timeo_p = current_timeo;
>>         }
>>  out:
>> -       remove_wait_queue(sk_sleep(sk), &wait);
>> +       if (!sock_flag(sk, SOCK_DEAD))
>> +               remove_wait_queue(sk_sleep(sk), &wait);
>>         return err;
>>
>>  do_error:
>>
>
> OK, but what about tcp_msg_wait_data() and udp_msg_wait_data () ?
> It seems they could be vulnerable as well..

TBH haven't thought about these until now.

They are reachable only via sk->sk_prot->recvmsg, so from recvmsg(2) or
recvmmsg(2). syscall layer will keep the socket file and the wait queue
alive.

[PATCH bpf-next 2/2] selftests/bpf: Add wait send memory test for sockmap redirect

[Liu Jian]

Add one test for wait redirect sock's send memory test for sockmap.

Signed-off-by: Liu Jian <liujian56@huawei.com>
---
 tools/testing/selftests/bpf/test_sockmap.c | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 0fbaccdc8861..95b9b45ad028 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -138,6 +138,7 @@ struct sockmap_options {
 	bool data_test;
 	bool drop_expected;
 	bool check_recved_len;
+	bool tx_wait_mem;
 	int iov_count;
 	int iov_length;
 	int rate;
@@ -578,6 +579,10 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 			sent = sendmsg(fd, &msg, flags);
 
 			if (!drop && sent < 0) {
+				if (opt->tx_wait_mem && errno == EACCES) {
+					errno = 0;
+					goto out_errno;
+				}
 				perror("sendmsg loop error");
 				goto out_errno;
 			} else if (drop && sent >= 0) {
@@ -644,6 +649,15 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 				goto out_errno;
 			}
 
+			if (opt->tx_wait_mem) {
+				FD_ZERO(&w);
+				FD_SET(fd, &w);
+				slct = select(max_fd + 1, NULL, NULL, &w, &timeout);
+				errno = 0;
+				close(fd);
+				goto out_errno;
+			}
+
 			errno = 0;
 			if (peek_flag) {
 				flags |= MSG_PEEK;
@@ -752,6 +766,22 @@ static int sendmsg_test(struct sockmap_options *opt)
 			return err;
 	}
 
+	if (opt->tx_wait_mem) {
+		struct timeval timeout;
+		int rxtx_buf_len = 1024;
+
+		timeout.tv_sec = 3;
+		timeout.tv_usec = 0;
+
+		err = setsockopt(c2, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(struct timeval));
+		err |= setsockopt(c2, SOL_SOCKET, SO_SNDBUFFORCE, &rxtx_buf_len, sizeof(int));
+		err |= setsockopt(p2, SOL_SOCKET, SO_RCVBUFFORCE, &rxtx_buf_len, sizeof(int));
+		if (err) {
+			perror("setsockopt failed()");
+			return errno;
+		}
+	}
+
 	rxpid = fork();
 	if (rxpid == 0) {
 		if (txmsg_pop || txmsg_start_pop)
@@ -788,6 +818,9 @@ static int sendmsg_test(struct sockmap_options *opt)
 		return errno;
 	}
 
+	if (opt->tx_wait_mem)
+		close(c2);
+
 	txpid = fork();
 	if (txpid == 0) {
 		if (opt->sendpage)
@@ -1452,6 +1485,14 @@ static void test_txmsg_redir(int cgrp, struct sockmap_options *opt)
 	test_send(opt, cgrp);
 }
 
+static void test_txmsg_redir_wait_sndmem(int cgrp, struct sockmap_options *opt)
+{
+	txmsg_redir = 1;
+	opt->tx_wait_mem = true;
+	test_send_large(opt, cgrp);
+	opt->tx_wait_mem = false;
+}
+
 static void test_txmsg_drop(int cgrp, struct sockmap_options *opt)
 {
 	txmsg_drop = 1;
@@ -1800,6 +1841,7 @@ static int populate_progs(char *bpf_file)
 struct _test test[] = {
 	{"txmsg test passthrough", test_txmsg_pass},
 	{"txmsg test redirect", test_txmsg_redir},
+	{"txmsg test redirect wait send mem", test_txmsg_redir_wait_sndmem},
 	{"txmsg test drop", test_txmsg_drop},
 	{"txmsg test ingress redirect", test_txmsg_ingress_redir},
 	{"txmsg test skb", test_txmsg_skb},
-- 
2.17.1