[PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
both types have member named "progs", the offset of "progs" member in
these two types is different, so "progs" should be accessed with the
real map type.

Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 net/core/sock_map.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 19538d628714..936c5cabe9f3 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -148,13 +148,13 @@ static void sock_map_del_link(struct sock *sk,
 	list_for_each_entry_safe(link, tmp, &psock->link, list) {
 		if (link->link_raw == link_raw) {
 			struct bpf_map *map = link->map;
-			struct bpf_stab *stab = container_of(map, struct bpf_stab,
-							     map);
-			if (psock->saved_data_ready && stab->progs.stream_parser)
+			struct sk_psock_progs *progs = sock_map_progs(map);
+
+			if (psock->saved_data_ready && progs->stream_parser)
 				strp_stop = true;
-			if (psock->saved_data_ready && stab->progs.stream_verdict)
+			if (psock->saved_data_ready && progs->stream_verdict)
 				verdict_stop = true;
-			if (psock->saved_data_ready && stab->progs.skb_verdict)
+			if (psock->saved_data_ready && progs->skb_verdict)
 				verdict_stop = true;
 			list_del(&link->list);
 			sk_psock_free_link(link);
-- 
2.30.2

Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[Martin KaFai Lau]

On 7/28/23 3:56 AM, Xu Kuohai wrote:
> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
> both types have member named "progs", the offset of "progs" member in
> these two types is different, so "progs" should be accessed with the
> real map type.

The patch makes sense to me. Can a test be written to trigger it?

John, please review.

Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[Xu Kuohai]

On 8/1/2023 9:19 AM, Martin KaFai Lau wrote:
> On 7/28/23 3:56 AM, Xu Kuohai wrote:
>> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
>> both types have member named "progs", the offset of "progs" member in
>> these two types is different, so "progs" should be accessed with the
>> real map type.
> 
> The patch makes sense to me. Can a test be written to trigger it?
> 

Thank you for the review. I have a messy prog that triggers memleak
caused by this issue. I'll try to simplify it to a test.

> John, please review.
> 
> 
> .

Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[John Fastabend]

Xu Kuohai wrote:
> On 8/1/2023 9:19 AM, Martin KaFai Lau wrote:
> > On 7/28/23 3:56 AM, Xu Kuohai wrote:
> >> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
> >> both types have member named "progs", the offset of "progs" member in
> >> these two types is different, so "progs" should be accessed with the
> >> real map type.
> > 
> > The patch makes sense to me. Can a test be written to trigger it?
> > 
> 
> Thank you for the review. I have a messy prog that triggers memleak
> caused by this issue. I'll try to simplify it to a test.
> 
> > John, please review.
> > 
> > 
> > .
> 
> 

Thanks good catch. One thing I don't see any tests for is deleting a
socket from a sockmap and then trying to use it? My guess is almost
no one deletes sockets from a map except on sock close. Maybe to
reproduce,

 1. connect a bunch of sockets to sockhash with verdict prog
 2. remove the sockets
 3. remove the sockhash
 4. that should leak the bpf ref cnt so we could check for the
    prog still existing?

Reviewed-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[Xu Kuohai]

On 8/1/2023 11:47 AM, John Fastabend wrote:
> Xu Kuohai wrote:
>> On 8/1/2023 9:19 AM, Martin KaFai Lau wrote:
>>> On 7/28/23 3:56 AM, Xu Kuohai wrote:
>>>> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
>>>> both types have member named "progs", the offset of "progs" member in
>>>> these two types is different, so "progs" should be accessed with the
>>>> real map type.
>>>
>>> The patch makes sense to me. Can a test be written to trigger it?
>>>
>>
>> Thank you for the review. I have a messy prog that triggers memleak
>> caused by this issue. I'll try to simplify it to a test.
>>
>>> John, please review.
>>>
>>>
>>> .
>>
>>
> 
> Thanks good catch. One thing I don't see any tests for is deleting a
> socket from a sockmap and then trying to use it? My guess is almost
> no one deletes sockets from a map except on sock close. Maybe to
> reproduce,
> 
>   1. connect a bunch of sockets to sockhash with verdict prog
>   2. remove the sockets
>   3. remove the sockhash
>   4. that should leak the bpf ref cnt so we could check for the
>      prog still existing?
> 

I tried this and found no bpf prog leaks. The debugging shows that
the stream_parser and stream_verdict progs are released as follows:

sock_map_unref

   sock_map_del_link

     struct bpf_stab *stab = container_of(map, struct bpf_stab, map);

     if (psock->saved_data_ready && stab->progs.stream_parser)
       strp_stop = true; // (1) not executed, since stab->progs.stream_parser
                         //     is actually shtab->progs.msg_parser, which is
                         //     NULL, so the if condition is false.

     if (psock->saved_data_ready && stab->progs.stream_verdict)
       verdict_stop = true;  // (2) executed, so verdict_stop is set to true

     if (strp_stop) // (3) condition is false since strp_stop is false
       sk_psock_stop_strp(sk, psock)

     if (verdict_stop) // (4) condition pass, so stream_verdict prog refcnt
                       //     is released by sk_psock_stop_verdict
       sk_psock_stop_verdict(sk, psock)
         psock_set_prog(&pock->progs.stream_verdict, NULL)
           bpf_prog_put // (5) release refcnt of stream_verdict prog


   sk_psock_put
       sk_psock_drop(sk, psock)
         sk_psock_stop_strp(sk, psock)
           sk_psock_stop_strp(&psock->progs.stream_parser, NULL)
             bpf_prog_put // (6) release refcnt of stream_parser prog



However, this issue triggers a WARNING in strp_done:

sock_map_unref
   sock_map_del_link

     struct bpf_stab *stab = container_of(map, struct bpf_stab, map);

     if (psock->saved_data_ready && stab->progs.stream_verdict)
       verdict_stop = true;  // (1) verdict_stop is set to true


     if (verdict_stop) // (2) condition pass
       sk_psock_stop_verdict(sk, psock)
         psock_set_prog(&pock->progs.stream_verdict, NULL)
           bpf_prog_put
         psock->saved_data_ready = NULL;  // (3) psock->saved_data_ready is
                                          //     set to NULL

   sk_psock_put
       sk_psock_drop(sk, psock)

         sk_psock_stop_strp(sk, psock)

           if (!psock->saved_data_ready) return; // (4) sk_psock_stop_strp returns

           strp_stop(&psock->strp) // (5) so strp_stop can not be called
             strp->stopped = 1; // (6) so strp->stopped is **NOT** set to 1

         sk_psock_destroy
           sk_psock_done_strp
             strp_done
               WARN_ON(!strp->stopped); // (7) WARNING triggered


Now I'm convinced the memleak I observed was caused by strp_done not
being called, I'll send a test for it.


> Reviewed-by: John Fastabend <john.fastabend@gmail.com>
> 
> 
> .

Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

[Jakub Sitnicki]

On Fri, Jul 28, 2023 at 06:56 AM -04, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
>
> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
> both types have member named "progs", the offset of "progs" member in
> these two types is different, so "progs" should be accessed with the
> real map type.
>
> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---

Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>