On Mon, Jun 18, 2018 at 07:50:19AM -0700, John Fastabend wrote:
> 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.
Make sense if there is no use case. It will be helpful if the commit log
is updated accordingly. Thanks!
Acked-by: Martin KaFai Lau <ka...@fb.com>
