From: John Fastabend <john.fastabend@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>,
Eric Dumazet <eric.dumazet@gmail.com>,
edumazet@google.com, ast@kernel.org,
Dave Watson <davejwatson@fb.com>
Cc: netdev@vger.kernel.org
Subject: Re: [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added
Date: Mon, 4 Jun 2018 06:57:28 -0700 [thread overview]
Message-ID: <aeff8901-2ca4-99b6-6238-93703946b233@gmail.com> (raw)
In-Reply-To: <c9eab906-6793-1e98-b9d8-01d665ac1c3c@iogearbox.net>
On 06/04/2018 06:39 AM, Daniel Borkmann wrote:
> Hey guys,
>
> On 06/02/2018 11:39 PM, John Fastabend wrote:
>> On 06/01/2018 12:58 PM, Eric Dumazet wrote:
>>> On 06/01/2018 03:46 PM, John Fastabend wrote:
>>>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>>>> of tcpv6_prot.
>>>
>>> ...
>>>
>>>> + /* ULPs are currently supported only for TCP sockets in ESTABLISHED
>>>> + * state. Supporting sockets in LISTEN state will require us to
>>>> + * modify the accept implementation to clone rather then share the
>>>> + * ulp context.
>>>> + */
>>>> + if (sock->sk_state != TCP_ESTABLISHED)
>>>> + return -ENOTSUPP;
>>>> +
>>>> /* 1. If sock map has BPF programs those will be inherited by the
>>>> * sock being added. If the sock is already attached to BPF programs
>>>> * this results in an error.
>>>
>>> Next question will be then : What happens if syzbot uses tcp_disconnect() and then listen() ?
>>
>> Yep we need to fix that as well :( Looks like we can plumb the
>> unhash callback and remove it from the sockmap when the socket
>> goes through tcp_disconnect().
>>
>> This patch should go in as-is though and we can fix the disconnect
>> issue with a new patch.
>>
>> Adding Dave Watson to the thread as well because I'm guessing
>> the disconnect() case is also applicable to TLS. At least I see
>> a hw handler for unhash but there does not appear to be a handler
>> in the SW case, at least from a quick glance.
>>
>> Thanks again!
>
> Given the discussion and fixes weren't resolved resp. ready in time for 4.17,
> and last bpf pr for it went out last week, we need to route this via -stable
> once all is hashed out.
>
OK.
> This fix here therefore needs to be rebased against bpf-next tree, and as far
> as I can see another fix for hash map is also needed to address the same issue.
>
This fix works for both sockmap and sockhash because they use the same
ulp register and init paths. But, will rebase for net-next and send out
this morning.
> After that, likely also fixes for the disconnect + listen case are needed.
>
Yep will have a fix today for this.
> (I can use the one here later on for -stable backport, but given merge window
> is open this needs a rebase and a resolution for hash map.)
>
hash map is also resolved with the same patch but please do queue this
up for -stable.
> Thanks,
> Daniel
>
next prev parent reply other threads:[~2018-06-04 13:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-01 19:46 [bpf PATCH v2] bpf: sockmap, fix crash when ipv6 sock is added John Fastabend
2018-06-01 19:58 ` Eric Dumazet
2018-06-02 21:39 ` John Fastabend
2018-06-04 13:39 ` Daniel Borkmann
2018-06-04 13:57 ` John Fastabend [this message]
2018-06-04 14:55 ` Daniel Borkmann
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=aeff8901-2ca4-99b6-6238-93703946b233@gmail.com \
--to=john.fastabend@gmail.com \
--cc=ast@kernel.org \
--cc=daniel@iogearbox.net \
--cc=davejwatson@fb.com \
--cc=edumazet@google.com \
--cc=eric.dumazet@gmail.com \
--cc=netdev@vger.kernel.org \
/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).