svn commit: r265232 - head/sys/net

[Alan Somers]

On Fri, May 2, 2014 at 3:08 PM, Alexander V. Chernikov
<melifaro at freebsd.org> wrote:
> On 03.05.2014 00:22, Alan Somers wrote:
>> On Fri, May 2, 2014 at 11:01 AM, Alexander V. Chernikov
>> <melifaro at freebsd.org> wrote:
>>> On 02.05.2014 20:24, Alan Somers wrote:
>>>>
>>>> Author: asomers
>>>> Date: Fri May  2 16:24:09 2014
>>>> New Revision: 265232
>>>> URL: http://svnweb.freebsd.org/changeset/base/265232
>>>>
>>>> Log:
>>>>    Fix a panic caused by doing "ifconfig -am" while a lagg is being
>>>> destroyed.
>>>>    The thread that is destroying the lagg has already set sc->sc_psc=NULL
>>>> when
>>>>    the "ifconfig -am" thread gets to lacp_req().  It tries to dereference
>>>>    sc->sc_psc and panics.  The solution is for lacp_req() to check the
>>>> value of
>>>>    sc->sc_psc.  If NULL, harmlessly return an lacp_opreq structure full of
>>>>    zeros.  Full details in GNATS.
>>>
>>> Sorry, it looks like I've missed -net@ discussion.
>>
>> Thanks for the retroactive review; it's good too.
>>
>>>
>>> While this patch minimizes panics, they still can occur.
>>> If one thread performs lagg_clone_destroy() -> lagg_lacp_detach() (1) ->
>>> lacp_detach(), executing
>>>
>>> struct lacp_softc *lsc = LACP_SOFTC(sc); (e.g. one line before sc_psc = NULL
>>> assignment)
>>>
>>> At the same moment, another thread (initiated by ifconfig) executes
>>>
>>>
>>> struct lacp_softc *lsc = LACP_SOFTC(sc);
>>>
>>> Then, thread #1 goes further to
>>>
>>> LACP_LOCK_DESTROY(lsc);
>>> free(lsc, M_DEVBUF);
>>>
>>> After that, thread #2 performs
>>>
>>> bzero(req, sizeof(struct lacp_opreq));
>>>
>>> passes lsc check for NULL and crashes on destroyed mutex.
>>>
>>>
>>> Briefly looking, we can remove WUNLOCK()  before lacp_detach() in
>>> lagg_lacp_detach() (I'm unsure about the reasons why do we need unlock
>>> here).
>>> lacp_req() is already protected by at least LAGG_RLOCK() so we should get
>>> consistent view of sc->sc_psc.
>>
>> The WUNLOCK was added in r168561 without comment.  Removing it seems
>> like it would work.  It doesn't cause any new LORs or WITNESS
>> warnings.  And I can no longer reproduce the panic in lacp_req.
>> However, it doesn't fix another panic in lagg_port_ioctl line 825 (my
>> patch didn't either).  Do you have any good ideas what to do about
>> that?
> Interesting.
> Line numbers look a bit shifted. Is line 825 'LAGG_RUNLOCK(sc, &tracker)' ?

Yes.

>
> Are the steps to reproduce it the same as in kern/189003?

Yes.  BTW, In kern/189003, I suggested reverting 253687.  In reality,
I didn't revert it; I commented out the sysctl lines it added in
lacp_attach().

>
>>
>> #9  0xffffffff808cee81 in __mtx_lock_sleep (c=0xfffff800052bb648,
>>     tid=18446735277704396800, opts=<value optimized out>,
>>     file=<value optimized out>, line=<value optimized out>)
>>     at /usr/home/alans/freebsd/head/sys/kern/kern_mutex.c:430
>> #10 0xffffffff808ced02 in __mtx_lock_flags (c=<value optimized out>, opts=0,
>>     file=0xffffffff80f6dd8c
>> "/usr/home/alans/freebsd/head/sys/kern/kern_rmlock.c", line=407) at
>> /usr/home/alans/freebsd/head/sys/kern/kern_mutex.c:223
>> #11 0xffffffff808e0032 in _rm_rlock (rm=0xfffff800052bb608,
>>     tracker=0xfffffe0097751918, trylock=<value optimized out>)
>>     at /usr/home/alans/freebsd/head/sys/kern/kern_rmlock.c:407
>> #12 0xffffffff808e0812 in _rm_rlock_debug (rm=0xfffff800052bb608,
>>     tracker=0xfffffe0097751918, trylock=0,
>>     file=0xffffffff81c1dd13
>> "/usr/home/alans/freebsd/head/sys/modules/if_lagg/../../net/if_lagg.c",
>> line=825)
>>     at /usr/home/alans/freebsd/head/sys/kern/kern_rmlock.c:659
>> #13 0xffffffff81c19f62 in lagg_port_ioctl (ifp=0xfffff8000590d800,
>>     cmd=<value optimized out>, data=0xfffff80005582c00 "tap0")
>>     at /usr/home/alans/freebsd/head/sys/modules/if_lagg/../../net/if_lagg.c:825
>> #14 0xffffffff809ae407 in ifioctl (so=<value optimized out>, cmd=3225971084,
>>     data=0xfffff80005582c00 "tap0", td=<value optimized out>)
>>     at /usr/home/alans/freebsd/head/sys/net/if.c:2643
>> #15 0xffffffff8093b9fb in kern_ioctl (td=<value optimized out>,
>>     fd=<value optimized out>, com=<value optimized out>) at file.h:323
>> #16 0xffffffff8093b77c in sys_ioctl (td=0xfffff800053cc000,
>>     uap=0xfffffe0097751b80)
>>     at /usr/home/alans/freebsd/head/sys/kern/sys_generic.c:702
>>
>