From: John Fastabend <john.fastabend@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Cc: "David S . Miller" <davem@davemloft.net>,
Alexei Starovoitov <ast@kernel.org>,
Shuah Khan <shuahkh@osg.samsung.com>,
netdev@vger.kernel.org
Subject: Re: [PATCH bpf] bpf: test_maps: cleanup sockmaps when test ends
Date: Wed, 24 Jan 2018 11:01:37 -0800 [thread overview]
Message-ID: <62856682-e82c-fcea-f39c-5ca4439533bd@gmail.com> (raw)
In-Reply-To: <df3fc194-cd03-0d96-9bf8-112aa283a74f@iogearbox.net>
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
prev parent reply other threads:[~2018-01-24 19:01 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=62856682-e82c-fcea-f39c-5ca4439533bd@gmail.com \
--to=john.fastabend@gmail.com \
--cc=alexei.starovoitov@gmail.com \
--cc=ast@kernel.org \
--cc=bhole_prashant_q7@lab.ntt.co.jp \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
--cc=shuahkh@osg.samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).