Hi Slawa,

On 10/12/16 11:52 AM, Slawa Olhovchenkov wrote:
> On Wed, Oct 12, 2016 at 11:42:38AM +0200, Julien Charbon wrote:
>> On 10/12/16 11:29 AM, Slawa Olhovchenkov wrote:
>>> On Wed, Oct 12, 2016 at 11:19:48AM +0200, Julien Charbon wrote:
>>>>> if INP_WLOCK is like spinlock -- this is dead lock.
>>>>> if INP_WLOCK is like mutex -- thread1 resheduled.
>>>>  Thanks, I understand you question now.  No an interrupt cannot bypass a
>>>> lock:  Here INP_WLOCK is like mutex -- thread1 resheduled.
>>> Thanks, nice.
>>>>>>> As I remeber race created by call tcp_twstart() at time of end
>>>>>>> tcp_close(), at path sofree()-tcp_usr_detach() and unexpected
>>>>>>> INP_TIMEWAIT state in the tcp_usr_detach(). INP_TIMEWAIT set in 
>>>>>>> tcp_twstart()
>>>>>>  Exactly, thus the current fix is:  If you already have the INP_DROPPED
>>>>>> flag set you are not allowed to call tcp_twstart(), actually it is a
>>>>>> good candidate for a new INVARIANT.  Let me add that.
>>>>>>> After check source code I am found invocation of tcp_twstart() in
>>>>>>> sys/netinet/tcp_stacks/fastpath.c, sys/netinet/tcp_input.c,
>>>>>>> sys/dev/cxgb/ulp/tom/cxgb_cpl_io.c, sys/dev/cxgbe/tom/t4_cpl_io.c.
>>>>>>> Invocation from sys/netinet/tcp_stacks/fastpath.c and
>>>>>>> sys/netinet/tcp_input.c guarded by INP_WLOCK in tcp_input(), and now
>>>>>>> will be OK.
>>>>>>> Invocation from sys/dev/cxgb/ulp/tom/cxgb_cpl_io.c and
>>>>>>> sys/dev/cxgbe/tom/t4_cpl_io.c is not clear to me, I am see independed
>>>>>>> INP_WLOCK. Is this OK?
>>>>>>> Can be thread A wants do_peer_close() directed from chelsio IRQ
>>>>>>> handler, bypass tcp_input()?
>>>>>>  If you look carefully INP_WLOCK is used in cxgb_cpl_io.c and
>>>>>> t4_cpl_io.c before calling tcp_twstart().
>>>>> Yes, and you remeber: sys/netinet/tcp_subr.c
>>>>>   1535  struct tcpcb *
>>>>>   1536  tcp_close(struct tcpcb *tp)
>>>>>   1537  {
>>>>> ...
>>>>>   1569                  INP_WUNLOCK(inp);
>>>>>   1570                  ACCEPT_LOCK();
>>>>>   1571                  SOCK_LOCK(so);
>>>>>   1572                  so->so_state &= ~SS_PROTOREF;
>>>>>   1573                  sofree(so);
>>>>>   1574                  return (NULL);
>>>>> sofree() call tcp_usr_detach() and in tcp_usr_detach() we have
>>>>> unexpected INP_TIMEWAIT.
>>>>  I see, thus just for the context:  The TCP stack in sys/dev/cxgb* is a
>>>> TOE (TCP Offload Engine?) TCP stack for Chelsio NICs, it is a
>>>> separate/side TCP stack that is used only with TCP_OFFLOAD option.
>>>>  This TOE TCP stack actually has its own set of detach()/input()
>>>> functions and seems to check INP_DROPPED flag properly.  I guess @np
>>>> check fixes in socket TCP stack and decides which one can also impact
>>>> the Chelsio TOE TCP stack.  Some bugs are only in socket TCP stack, some
>>>> are only in TOE TCP stack.
>>> I am fear about other direction -- setting INP_TIMEWAIT in Chelsio TOE
>>> TCP stack and impact this to
>>> tcp_timer_2msl()/tcp_close()/sofree()/tcp_usr_detach() path.
>>  I see, I expect no problem on this side as tcp_timer_2msl() checks the
>> INP_TIMEWAIT flag and do not call tcp_close() if set.
> I am about case when at time of first INP_WUNLOCK() tcp_timer_2msl()
> don't see INP_TIMEWAIT, call tcp_close(), tcp_close() do INP_WUNLOCK()
> and now Chelsio TOE take INP_WLOCK, do tcp_twstart() and set
> INP_TIMEWAIT. After this tcp_timer_2msl resume and have unexpected
> INP_TIMEWAIT in tcp_usr_detach().

 Sure, basically the same bug that in classic TCP stack.  If you think
it can happen, send an email describing that to np@ and he will check
and fix that.  He is a TOE TCP stack expert and I am not.  In all cases,
if this issue is possible in TOE TCP stack context, the patch will be
straightforward:  If the INP_DROPPED flag is set do not call tcp_twstart().

 The current patch focuses only on the classic TCP stack.


Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to