Date: Fri, 5 Jun 2015 18:54:18 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Sean Bruno <sbruno@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284029 - head/sys/kern Message-ID: <20150605165417.GA13205@dft-labs.eu> In-Reply-To: <201506051621.t55GLiKp095005@svn.freebsd.org> References: <201506051621.t55GLiKp095005@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 05, 2015 at 04:21:44PM +0000, Sean Bruno wrote: > Author: sbruno > Date: Fri Jun 5 16:21:43 2015 > New Revision: 284029 > URL: https://svnweb.freebsd.org/changeset/base/284029 > > Log: > This change uses a reader count instead of holding the mutex for the > interpreter list to avoid the problem of holding a non-sleep lock during > a page fault as reported by witness. In addition, it consistently uses > memset()/memcpy() instead of bzero()/bcopy() except in the case where > bcopy() is required (i.e. overlapping copy). What lock was that? If it has to be held, why not use an sx lock? The change looks quite buggy. > > Differential Revision: https://reviews.freebsd.org/D2123 > Submitted by: sson > MFC after: 2 weeks > Relnotes: Yes > > Modified: > head/sys/kern/imgact_binmisc.c > > Modified: head/sys/kern/imgact_binmisc.c > ============================================================================== > --- head/sys/kern/imgact_binmisc.c Fri Jun 5 16:02:24 2015 (r284028) > +++ head/sys/kern/imgact_binmisc.c Fri Jun 5 16:21:43 2015 (r284029) > @@ -1,5 +1,5 @@ > /*- > - * Copyright (c) 2013, Stacey D. Son > + * Copyright (c) 2013-15, Stacey D. Son > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > @@ -41,6 +41,9 @@ __FBSDID("$FreeBSD$"); > #include <sys/malloc.h> > #include <sys/mutex.h> > #include <sys/sysctl.h> > +#include <sys/sx.h> > + > +#include <machine/atomic.h> > > /** > * Miscellaneous binary interpreter image activator. > @@ -95,11 +98,68 @@ static SLIST_HEAD(, imgact_binmisc_entry > SLIST_HEAD_INITIALIZER(interpreter_list); > > static int interp_list_entry_count = 0; > - > +static int interp_list_rcount = 0; > static struct mtx interp_list_mtx; > > int imgact_binmisc_exec(struct image_params *imgp); > > +static inline void > +interp_list_rlock() > +{ > + > + /* Don't use atomic_add() here. We must block if the mutex is held. */ > + mtx_lock(&interp_list_mtx); > + interp_list_rcount++; > + mtx_unlock(&interp_list_mtx); > +} > + > +static inline void > +interp_list_runlock() > +{ > + > + /* Decrement the reader count and wake up one writer, if any. */ > + atomic_subtract_int(&interp_list_rcount, 1); > + if (interp_list_rcount == 0) > + wakeup_one(&interp_list_rcount); > +} > + interp_list_rcount manipulation between these 2 is not safe. This effectively is: A B interp_list_rcount++ atomic_subtract_int(&interp_list_rcount, 1); As such updates can be lost. There is also potential for bonus wakeup. > +static inline void > +interp_list_wlock() > +{ > + > + /* Wait until there are no readers. */ > + mtx_lock(&interp_list_mtx); > + while (interp_list_rcount) > + mtx_sleep(&interp_list_rcount, &interp_list_mtx, 0, "IABM", 0); > +} > + Given that runlock does not take the lock, this func can sleep after wakeup_one() thus blocking the thread with nobody to wake it up. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150605165417.GA13205>