Re: panic with tcp timers

 Hi Randall,

On 6/25/16 4:41 PM, Randall Stewart via freebsd-net wrote:
> Ok
> 
> Lets try this again with my source changed to my _at_freebsd.net :-)
> 
> Now I am also attaching a patch for you Gleb, this will take some poking to
> get in to your NF-head since it incorporates some changes we made earlier.
> 
> I think this will fix the problem.. i.e. dealing with two locks in the callout system (which it was
> never meant to have done)..
> 
> Note we probably can move the code to use the callout lock init now.. but lets see if this works
> on your setup on c096 and if so we can think about doing that.

 Thanks for proposing a patch.  I believe your patch will work with
callout lock init, but not without:  You still have a use-after-free
issue on the tcpcb without callout lock init.

 The case being subtle as usual, let me try to describe that could happen:

 With your patch we have:

void
tcp_timer_keep(void *xtp)
{
        struct tcpcb *tp = xtp;
        struct tcptemp *t_template;
        struct inpcb *inp;
        CURVNET_SET(tp->t_vnet);
#ifdef TCPDEBUG
        int ostate;

        ostate = tp->t_state;
#endif
        inp = tp->t_inpcb;
        KASSERT(inp != NULL, ("%s: tp %p tp->t_inpcb == NULL", __func__,
tp));
        INP_WLOCK(inp);
        if (callout_pending(&tp->t_timers->tt_keep) ### Use after free
of tp here
            !callout_active(&tp->t_timers->tt_keep)) {
                INP_WUNLOCK(inp);
                CURVNET_RESTORE();
                return;
        }
        ...

 The use-after-free scenario:

[CPU 1] the callout fires, tcp_timer_keep entered
[CPU 1] blocks on INP_WLOCK(inp);
[CPU 2] schedules tcp_timer_keep with callout_reset()
[CPU 2] tcp_discardcb called
[CPU 2] tcp_timer_keep callout successfully canceled
[CPU 2] tcpcb freed
[CPU 1] unblocks, the tcpcb is used

 Then the tcpcb will used just after being freed...  Might also crash or
not depending in the case.

 Extra notes:

 o The invariant I see here is:  The "callout successfully canceled"
step should never happen when "the callout is currently being executed".

 o Solutions I see to enforce this invariant:

 - First solution:  Use callout lock init with inp lock, your patch
seems to permit that now.

 - Second solution:  Change callout_async_drain() behavior:  It can
return 0 (fail) when the callout is currently being executed (no matter
what).

 - Third solution:  Don't trust callout_async_drain(callout) return
value of 1 (success) if the previous call of callout_reset(callout)
returned 0 (fail).  That was the exact purpose of r284261 change, but
this solution is also step backward in modernization of TCP
timers/callout...

https://svnweb.freebsd.org/base/stable/10/sys/netinet/tcp_timer.c?r1=284261&r2=284260&pathrev=284261

 Hopefully my description is clear enough...

--
Julien