Can tcp_close() be called in INP_TIMEWAIT case

[Julien Charbon]

 Hi Hiren,

On 26/09/15 00:46, hiren panchasara wrote:
> On 09/25/15 at 09:42P, John Baldwin wrote:
>> On Thursday, September 24, 2015 02:13:24 PM Julien Charbon wrote:
>>>  So the issue is:
>>>
>>>  - tcp_close() is called for some reasons with an inp in INP_TIMEWAIT
>>> state and sets the INP_DROPPED flag,
>>>  - tcp_detach() is called when the last reference on socket is dropped
>>>
>>>  then now in_pcbfree() can be called twice instead of once:
>>>
>>>  1. First in tcp_detach():
>>>
>>> static void
>>> tcp_detach(struct socket *so, struct inpcb *inp)
>>> {
>>>         struct tcpcb *tp;
>>>         tp = intotcpcb(inp);
>>>
>>>         if (inp->inp_flags & INP_TIMEWAIT) {
>>>                 if (inp->inp_flags & INP_DROPPED) {
>>>                         in_pcbdetach(inp);
>>>                         in_pcbfree(inp); <--
>>>                 }
>>>
>>>  2. Second when tcptw expires here:
>>>
>>> void
>>> tcp_twclose(struct tcptw *tw, int reuse)
>>> {
>>>         struct socket *so;
>>>         struct inpcb *inp;
>>>
>>>         inp = tw->tw_inpcb;
>>>
>>>         tcp_tw_2msl_stop(tw, reuse);
>>>         inp->inp_ppcb = NULL;
>>>         in_pcbdrop(inp);
>>>
>>>         so = inp->inp_socket;
>>>         if (so != NULL) {
>>>                 ...
>>>         } else {
>>>                 in_pcbfree(inp); <--
>>>         }
>>>
>>>  This behavior is backed by Palle kernel panic backstraces and coredumps.
>>>
>>>  o Solutions:
>>>
>>>  Long:  Forbid to call tcp_close() when inp is in INP_TIMEWAIT state,
>>> the TCP stack rule being:
>>>
>>>  - if !INP_TIMEWAIT: Call tcp_close()
>>>  - if INP_TIMEWAIT: Call tcp_twclose() (or call nothing, the tcptw will
>>> expire/be recycled anyway)
>>>
>>>  Short:
>>>   if INP_TIMEWAIT & INP_DROPPED:
>>>     Do not call in_pcbfree(inp) in tcp_detach() unless tcptw is already
>>> discarded.
>>>
>>>  The long solution seems cleaner, backed by tcp_detach() old comments
>>> and most of current tcp_close() calls but I won't take that longer path
>>> without -net approval first.
>>
>> I prefer the longer solution if it keeps tcp_detach() simpler by avoiding
>> an extra condition.  Please just document it via assertions in tcp_close()
>> (or is this the assertion that fired and triggered the reported panic?  If so,
>> then you obviously don't need to add it. :-P)
> 
> I also like the longer solution because it seems cleaner and more
> readable/followable.

 Thanks for your answer.  Will do this change and create a review for it.

> Julien, nice work on investigation and follow-up. :-)

 Thanks!

--
Julien

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20150928/985a7193/attachment.bin>