[prev in list] [next in list] [prev in thread] [next in thread]
List: freebsd-net
Subject: Re: EBR fix for life cycle races was Re: panic with tcp timers
From: Matthew Macy <mmacy () nextbsd ! org>
Date: 2016-06-30 8:41:34
Message-ID: 155a0786ddb.11b26639c42755.8788446779628644237 () nextbsd ! org
[Download RAW message or body]
---- On Tue, 28 Jun 2016 23:19:45 -0700 Matthew Macy <mmacy@nextbsd.org> wrote ----
>
>
>
> ---- On Tue, 28 Jun 2016 15:51:57 -0700 K. Macy <kmacy@freebsd.org> wrote ----
> > On Tue, Jun 28, 2016 at 10:51 AM, Matthew Macy <mmacy@nextbsd.org> wrote:
> > > You guys should really look at Samy Bahra's epoch based reclamation. I solved \
a similar problem in drm/linuxkpi using it. > >
> > The point being that this is a bug in the TCP life cycle handling
> > _not_ in callouts. Churning the callout interface is not the best /
> > only solution.
> > -M
>
> Please see see D7017/D7018 as an example for an ultimately more robust solution \
than continued fiddling with the callout interface. >
> https://reviews.freebsd.org/D7018
I realized that this shortens the race but still leaves one open from the time the \
callout lock is dropped to the time the epoch begins. I have a proposed fix to make \
read locks never block and to essentially close the race window.
The next issue that comes up is that synchronize is called too often. I'll talk to \
Samy about it in a few hours and come up with a better design.
-M
>
> > > ---- On Tue, 28 Jun 2016 02:58:56 -0700 Julien Charbon <jch@freebsd.org> \
wrote ---- > > > >
> > > > 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 @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
> > > >
> > > >
> > >
> > > _______________________________________________
> > > freebsd-current@freebsd.org mailing list
> > > https://lists.freebsd.org/mailman/listinfo/freebsd-current
> > > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
> > _______________________________________________
> > freebsd-current@freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/freebsd-current
> > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
> >
>
> _______________________________________________
> freebsd-current@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
>
_______________________________________________
freebsd-net@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic