svn commit: r265232 - head/sys/net

[Alexander V. Chernikov]

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.

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.

>    
>    PR:		kern/189003
>    Reviewed by:	timeout on freebsd-net@
>    MFC after:	3 weeks
>    Sponsored by:	Spectra Logic Corporation
>
> Modified:
>    head/sys/net/ieee8023ad_lacp.c
>
> Modified: head/sys/net/ieee8023ad_lacp.c
> ==============================================================================
> --- head/sys/net/ieee8023ad_lacp.c	Fri May  2 16:15:34 2014	(r265231)
> +++ head/sys/net/ieee8023ad_lacp.c	Fri May  2 16:24:09 2014	(r265232)
> @@ -590,10 +590,20 @@ lacp_req(struct lagg_softc *sc, caddr_t
>   {
>   	struct lacp_opreq *req = (struct lacp_opreq *)data;
>   	struct lacp_softc *lsc = LACP_SOFTC(sc);
> -	struct lacp_aggregator *la = lsc->lsc_active_aggregator;
> +	struct lacp_aggregator *la;
>   
> -	LACP_LOCK(lsc);
>   	bzero(req, sizeof(struct lacp_opreq));
> +	
> +	/*
> +	 * If the LACP softc is NULL, return with the opreq structure full of
> +	 * zeros.  It is normal for the softc to be NULL while the lagg is
> +	 * being destroyed.
> +	 */
> +	if (NULL == lsc)
> +		return;
> +
> +	la = lsc->lsc_active_aggregator;
> +	LACP_LOCK(lsc);
>   	if (la != NULL) {
>   		req->actor_prio = ntohs(la->la_actor.lip_systemid.lsi_prio);
>   		memcpy(&req->actor_mac, &la->la_actor.lip_systemid.lsi_mac,
>
>