From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [bpf PATCH v2 2/6] bpf: sockmap only allow ESTABLISHED sock state Date: Mon, 18 Jun 2018 07:50:19 -0700 Message-ID: <783d9ac4-3291-04cf-98a7-a05f31a833e4@gmail.com> References: <20180614164148.24994.65250.stgit@john-Precision-Tower-5810> <20180614164451.24994.31096.stgit@john-Precision-Tower-5810> <20180615001856.dc2huljl7o554vzn@kafai-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org To: Martin KaFai Lau Return-path: Received: from mail-io0-f193.google.com ([209.85.223.193]:43308 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934674AbeFROvC (ORCPT ); Mon, 18 Jun 2018 10:51:02 -0400 Received: by mail-io0-f193.google.com with SMTP id t6-v6so17035600iob.10 for ; Mon, 18 Jun 2018 07:51:02 -0700 (PDT) In-Reply-To: <20180615001856.dc2huljl7o554vzn@kafai-mbp.dhcp.thefacebook.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/14/2018 05:18 PM, Martin KaFai Lau wrote: > On Thu, Jun 14, 2018 at 09:44:52AM -0700, John Fastabend wrote: >> Per the note in the TLS ULP (which is actually a generic statement >> regarding ULPs) >> >> /* The TLS ulp is 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. >> */ > Can you explain how that apply to bpf_tcp ulp? > > My understanding is the "ulp context" referred in TLS ulp is > the tls_context stored in icsk_ulp_data but I don't see bpf_tcp's > ulp is using icsk_ulp_data. > > Others LGTM. > So I think you are right we could probably allow it here but I am thinking I'll leave the check for now anyways for a couple reasons. First, we will shortly add support to allow ULP types to coexist. At the moment the two ULP types can not coexist. When this happens it looks like we will need to restrict to only ESTABLISHED types or somehow make all ULPs work in all states. Second, I don't have any use cases (nor can I think of any) for the sock{map|hash} ULP to be running on a non ESTABLISHED socket. Its not clear to me that having the sendmsg/sendpage hooks for a LISTEN socket makes sense. I would rather restrict it now and if we add something later where it makes sense to run on non-ESTABLISHED socks we can remove the check. Thanks for reviewing, John