netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
@ 2018-01-23  4:30 Prashant Bhole
  2018-01-23  4:38 ` Alexei Starovoitov
  0 siblings, 1 reply; 7+ messages in thread
From: Prashant Bhole @ 2018-01-23  4:30 UTC (permalink / raw)
  To: David S . Miller, Alexei Starovoitov, Daniel Borkmann
  Cc: Prashant Bhole, Shuah Khan, netdev

Bug: BPF programs and maps related to sockmaps test exist in
memory even after test_maps ends

This patch fixes it by empyting sockmaps when test ends.

Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_maps.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 040356ecc862..5fb1355f80fc 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -463,7 +463,7 @@ static void test_devmap(int task, void *data)
 #define SOCKMAP_VERDICT_PROG "./sockmap_verdict_prog.o"
 static void test_sockmap(int tasks, void *data)
 {
-	int one = 1, map_fd_rx, map_fd_tx, map_fd_break, s, sc, rc;
+	int one = 1, map_fd_rx = 0, map_fd_tx = 0, map_fd_break, s, sc, rc;
 	struct bpf_map *bpf_map_rx, *bpf_map_tx, *bpf_map_break;
 	int ports[] = {50200, 50201, 50202, 50204};
 	int err, i, fd, udp, sfd[6] = {0xdeadbeef};
@@ -868,9 +868,12 @@ static void test_sockmap(int tasks, void *data)
 		goto out_sockmap;
 	}
 
-	/* Test map close sockets */
-	for (i = 0; i < 6; i++)
+	/* Test map close sockets and empty maps */
+	for (i = 0; i < 6; i++) {
+		bpf_map_delete_elem(map_fd_tx, &i);
+		bpf_map_delete_elem(map_fd_rx, &i);
 		close(sfd[i]);
+	}
 	close(fd);
 	close(map_fd_rx);
 	bpf_object__close(obj);
@@ -881,8 +884,13 @@ static void test_sockmap(int tasks, void *data)
 	printf("Failed to create sockmap '%i:%s'!\n", i, strerror(errno));
 	exit(1);
 out_sockmap:
-	for (i = 0; i < 6; i++)
+	for (i = 0; i < 6; i++) {
+		if (map_fd_tx)
+			bpf_map_delete_elem(map_fd_tx, &i);
+		if (map_fd_rx)
+			bpf_map_delete_elem(map_fd_rx, &i);
 		close(sfd[i]);
+	}
 	close(fd);
 	exit(1);
 }
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
  2018-01-23  4:30 [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends Prashant Bhole
@ 2018-01-23  4:38 ` Alexei Starovoitov
  2018-01-23  4:55   ` Prashant Bhole
  0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2018-01-23  4:38 UTC (permalink / raw)
  To: Prashant Bhole
  Cc: David S . Miller, Alexei Starovoitov, Daniel Borkmann,
	Shuah Khan, netdev, John Fastabend

On Tue, Jan 23, 2018 at 01:30:44PM +0900, Prashant Bhole wrote:
> Bug: BPF programs and maps related to sockmaps test exist in
> memory even after test_maps ends
> 
> This patch fixes it by empyting sockmaps when test ends.
> 
> Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>

that's a fine workaround and I'm planning to apply this patch
to bpf-next, but it's not a fix. The sockmap should have cleaned
itself up.

> ---
>  tools/testing/selftests/bpf/test_maps.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> index 040356ecc862..5fb1355f80fc 100644
> --- a/tools/testing/selftests/bpf/test_maps.c
> +++ b/tools/testing/selftests/bpf/test_maps.c
> @@ -463,7 +463,7 @@ static void test_devmap(int task, void *data)
>  #define SOCKMAP_VERDICT_PROG "./sockmap_verdict_prog.o"
>  static void test_sockmap(int tasks, void *data)
>  {
> -	int one = 1, map_fd_rx, map_fd_tx, map_fd_break, s, sc, rc;
> +	int one = 1, map_fd_rx = 0, map_fd_tx = 0, map_fd_break, s, sc, rc;
>  	struct bpf_map *bpf_map_rx, *bpf_map_tx, *bpf_map_break;
>  	int ports[] = {50200, 50201, 50202, 50204};
>  	int err, i, fd, udp, sfd[6] = {0xdeadbeef};
> @@ -868,9 +868,12 @@ static void test_sockmap(int tasks, void *data)
>  		goto out_sockmap;
>  	}
>  
> -	/* Test map close sockets */
> -	for (i = 0; i < 6; i++)
> +	/* Test map close sockets and empty maps */
> +	for (i = 0; i < 6; i++) {
> +		bpf_map_delete_elem(map_fd_tx, &i);
> +		bpf_map_delete_elem(map_fd_rx, &i);
>  		close(sfd[i]);
> +	}
>  	close(fd);
>  	close(map_fd_rx);
>  	bpf_object__close(obj);
> @@ -881,8 +884,13 @@ static void test_sockmap(int tasks, void *data)
>  	printf("Failed to create sockmap '%i:%s'!\n", i, strerror(errno));
>  	exit(1);
>  out_sockmap:
> -	for (i = 0; i < 6; i++)
> +	for (i = 0; i < 6; i++) {
> +		if (map_fd_tx)
> +			bpf_map_delete_elem(map_fd_tx, &i);
> +		if (map_fd_rx)
> +			bpf_map_delete_elem(map_fd_rx, &i);
>  		close(sfd[i]);
> +	}
>  	close(fd);
>  	exit(1);
>  }
> -- 
> 2.13.6
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
  2018-01-23  4:38 ` Alexei Starovoitov
@ 2018-01-23  4:55   ` Prashant Bhole
  2018-01-23  5:01     ` Alexei Starovoitov
  0 siblings, 1 reply; 7+ messages in thread
From: Prashant Bhole @ 2018-01-23  4:55 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S . Miller, Alexei Starovoitov, Daniel Borkmann,
	Shuah Khan, netdev, John Fastabend



On 1/23/2018 1:38 PM, Alexei Starovoitov wrote:
> On Tue, Jan 23, 2018 at 01:30:44PM +0900, Prashant Bhole wrote:
>> Bug: BPF programs and maps related to sockmaps test exist in
>> memory even after test_maps ends
>>
>> This patch fixes it by empyting sockmaps when test ends.
>>
>> Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> 
> that's a fine workaround and I'm planning to apply this patch
> to bpf-next, but it's not a fix. The sockmap should have cleaned
> itself up.

Ok. Do I need to re-submit it targeted to -bpf-next and without fixes tag?

-Prashant
> 
>> ---
>>   tools/testing/selftests/bpf/test_maps.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
>> index 040356ecc862..5fb1355f80fc 100644
>> --- a/tools/testing/selftests/bpf/test_maps.c
>> +++ b/tools/testing/selftests/bpf/test_maps.c
>> @@ -463,7 +463,7 @@ static void test_devmap(int task, void *data)
>>   #define SOCKMAP_VERDICT_PROG "./sockmap_verdict_prog.o"
>>   static void test_sockmap(int tasks, void *data)
>>   {
>> -	int one = 1, map_fd_rx, map_fd_tx, map_fd_break, s, sc, rc;
>> +	int one = 1, map_fd_rx = 0, map_fd_tx = 0, map_fd_break, s, sc, rc;
>>   	struct bpf_map *bpf_map_rx, *bpf_map_tx, *bpf_map_break;
>>   	int ports[] = {50200, 50201, 50202, 50204};
>>   	int err, i, fd, udp, sfd[6] = {0xdeadbeef};
>> @@ -868,9 +868,12 @@ static void test_sockmap(int tasks, void *data)
>>   		goto out_sockmap;
>>   	}
>>   
>> -	/* Test map close sockets */
>> -	for (i = 0; i < 6; i++)
>> +	/* Test map close sockets and empty maps */
>> +	for (i = 0; i < 6; i++) {
>> +		bpf_map_delete_elem(map_fd_tx, &i);
>> +		bpf_map_delete_elem(map_fd_rx, &i);
>>   		close(sfd[i]);
>> +	}
>>   	close(fd);
>>   	close(map_fd_rx);
>>   	bpf_object__close(obj);
>> @@ -881,8 +884,13 @@ static void test_sockmap(int tasks, void *data)
>>   	printf("Failed to create sockmap '%i:%s'!\n", i, strerror(errno));
>>   	exit(1);
>>   out_sockmap:
>> -	for (i = 0; i < 6; i++)
>> +	for (i = 0; i < 6; i++) {
>> +		if (map_fd_tx)
>> +			bpf_map_delete_elem(map_fd_tx, &i);
>> +		if (map_fd_rx)
>> +			bpf_map_delete_elem(map_fd_rx, &i);
>>   		close(sfd[i]);
>> +	}
>>   	close(fd);
>>   	exit(1);
>>   }
>> -- 
>> 2.13.6
>>
>>
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
  2018-01-23  4:55   ` Prashant Bhole
@ 2018-01-23  5:01     ` Alexei Starovoitov
  2018-01-23  5:18       ` John Fastabend
  0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2018-01-23  5:01 UTC (permalink / raw)
  To: Prashant Bhole
  Cc: David S . Miller, Alexei Starovoitov, Daniel Borkmann,
	Shuah Khan, netdev, John Fastabend

On Tue, Jan 23, 2018 at 01:55:30PM +0900, Prashant Bhole wrote:
> 
> 
> On 1/23/2018 1:38 PM, Alexei Starovoitov wrote:
> > On Tue, Jan 23, 2018 at 01:30:44PM +0900, Prashant Bhole wrote:
> > > Bug: BPF programs and maps related to sockmaps test exist in
> > > memory even after test_maps ends
> > > 
> > > This patch fixes it by empyting sockmaps when test ends.
> > > 
> > > Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
> > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > 
> > that's a fine workaround and I'm planning to apply this patch
> > to bpf-next, but it's not a fix. The sockmap should have cleaned
> > itself up.
> 
> Ok. Do I need to re-submit it targeted to -bpf-next and without fixes tag?

No need. It's fine.

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
  2018-01-23  5:01     ` Alexei Starovoitov
@ 2018-01-23  5:18       ` John Fastabend
  2018-01-23 18:16         ` Daniel Borkmann
  0 siblings, 1 reply; 7+ messages in thread
From: John Fastabend @ 2018-01-23  5:18 UTC (permalink / raw)
  To: Alexei Starovoitov, Prashant Bhole
  Cc: David S . Miller, Alexei Starovoitov, Daniel Borkmann,
	Shuah Khan, netdev

On 01/22/2018 09:01 PM, Alexei Starovoitov wrote:
> On Tue, Jan 23, 2018 at 01:55:30PM +0900, Prashant Bhole wrote:
>>
>>
>> On 1/23/2018 1:38 PM, Alexei Starovoitov wrote:
>>> On Tue, Jan 23, 2018 at 01:30:44PM +0900, Prashant Bhole wrote:
>>>> Bug: BPF programs and maps related to sockmaps test exist in
>>>> memory even after test_maps ends
>>>>
>>>> This patch fixes it by empyting sockmaps when test ends.
>>>>
>>>> Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>
>>> that's a fine workaround and I'm planning to apply this patch
>>> to bpf-next, but it's not a fix. The sockmap should have cleaned
>>> itself up.
>>
>> Ok. Do I need to re-submit it targeted to -bpf-next and without fixes tag?
> 
> No need. It's fine.
> 

Also I'm looking over sockmap code now for the bug, should have
something shortly.

Agree this is a nice cleanup of the test code though. On the other
hand I should add some explicit tests for this case (deleting map
with elements) as well though.

.John

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
  2018-01-23  5:18       ` John Fastabend
@ 2018-01-23 18:16         ` Daniel Borkmann
  2018-01-24 19:01           ` John Fastabend
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Borkmann @ 2018-01-23 18:16 UTC (permalink / raw)
  To: John Fastabend, Alexei Starovoitov, Prashant Bhole
  Cc: David S . Miller, Alexei Starovoitov, Shuah Khan, netdev

On 01/23/2018 06:18 AM, John Fastabend wrote:
> On 01/22/2018 09:01 PM, Alexei Starovoitov wrote:
>> On Tue, Jan 23, 2018 at 01:55:30PM +0900, Prashant Bhole wrote:
>>> On 1/23/2018 1:38 PM, Alexei Starovoitov wrote:
>>>> On Tue, Jan 23, 2018 at 01:30:44PM +0900, Prashant Bhole wrote:
>>>>> Bug: BPF programs and maps related to sockmaps test exist in
>>>>> memory even after test_maps ends
>>>>>
>>>>> This patch fixes it by empyting sockmaps when test ends.
>>>>>
>>>>> Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
>>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>>
>>>> that's a fine workaround and I'm planning to apply this patch
>>>> to bpf-next, but it's not a fix. The sockmap should have cleaned
>>>> itself up.

Agree, this definitely needs kernel side fixing.

>>> Ok. Do I need to re-submit it targeted to -bpf-next and without fixes tag?
>>
>> No need. It's fine.
> 
> Also I'm looking over sockmap code now for the bug, should have
> something shortly.
> 
> Agree this is a nice cleanup of the test code though. On the other
> hand I should add some explicit tests for this case (deleting map
> with elements) as well though.

Ok, thanks for looking into fixing it! The latter could be added along
with test case for the fix.

I've tested and applied Prashant's patch to bpf-next with a note that
real fix is still TBD. Thanks for catching Prashant!

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
  2018-01-23 18:16         ` Daniel Borkmann
@ 2018-01-24 19:01           ` John Fastabend
  0 siblings, 0 replies; 7+ messages in thread
From: John Fastabend @ 2018-01-24 19:01 UTC (permalink / raw)
  To: Daniel Borkmann, Alexei Starovoitov, Prashant Bhole
  Cc: David S . Miller, Alexei Starovoitov, Shuah Khan, netdev

On 01/23/2018 10:16 AM, Daniel Borkmann wrote:
> On 01/23/2018 06:18 AM, John Fastabend wrote:
>> On 01/22/2018 09:01 PM, Alexei Starovoitov wrote:
>>> On Tue, Jan 23, 2018 at 01:55:30PM +0900, Prashant Bhole wrote:
>>>> On 1/23/2018 1:38 PM, Alexei Starovoitov wrote:
>>>>> On Tue, Jan 23, 2018 at 01:30:44PM +0900, Prashant Bhole wrote:
>>>>>> Bug: BPF programs and maps related to sockmaps test exist in
>>>>>> memory even after test_maps ends
>>>>>>
>>>>>> This patch fixes it by empyting sockmaps when test ends.
>>>>>>
>>>>>> Fixes: 6f6d33f3b3d0f ("bpf: selftests add sockmap tests")
>>>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>>>
>>>>> that's a fine workaround and I'm planning to apply this patch
>>>>> to bpf-next, but it's not a fix. The sockmap should have cleaned
>>>>> itself up.
> 
> Agree, this definitely needs kernel side fixing.
> 
>>>> Ok. Do I need to re-submit it targeted to -bpf-next and without fixes tag?
>>>
>>> No need. It's fine.
>>
>> Also I'm looking over sockmap code now for the bug, should have
>> something shortly.
>>
>> Agree this is a nice cleanup of the test code though. On the other
>> hand I should add some explicit tests for this case (deleting map
>> with elements) as well though.
> 
> Ok, thanks for looking into fixing it! The latter could be added along
> with test case for the fix.
> 

The issue is sockmap expects sockets to go through CLOSED state so
that we can cleanup any sockets the user forgot to remove. This is
required because of refcounts the psock struct (the socket representation
in sockmap) holds.

However, sockets in the LISTEN sk_state will not go through the CLOSED
state. So we never get a sk_state_change event and we never dec the
refcnt which allows the prog and then map to be released. In the test_maps
case I simply walked the entire set of socket descriptors and added them
all to the map, more out of laziness than anything.

The fix is to not allow sockets in the LISTEN state to be added to a
sockmap and also remove any sockets that transition into the LISTEN
state. Should have a fix shortly with some tests.

> I've tested and applied Prashant's patch to bpf-next with a note that
> real fix is still TBD. Thanks for catching Prashant!
> 

+1 Nice catch Prashant.

Thanks,
John

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-24 19:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-23  4:30 [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends Prashant Bhole
2018-01-23  4:38 ` Alexei Starovoitov
2018-01-23  4:55   ` Prashant Bhole
2018-01-23  5:01     ` Alexei Starovoitov
2018-01-23  5:18       ` John Fastabend
2018-01-23 18:16         ` Daniel Borkmann
2018-01-24 19:01           ` John Fastabend

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).