Re: [bpf PATCH v2 2/6] bpf: sockmap only allow ESTABLISHED sock state

[John Fastabend <john.fastabend@gmail.com>]

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