netdev - Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends

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] [day] [month] [year] [list]

Date:   Wed, 24 Jan 2018 11:01:37 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Shuah Khan <shuahkh@....samsung.com>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends

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@....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