* [Bug 11824][PATCH] ieee1394: raw1394: fix possible deadlock in multithreaded clients [not found] ` <4902F41E.5070306@s5r6.in-berlin.de> @ 2008-10-26 11:02 ` Stefan Richter 2008-10-27 10:13 ` Ingo Molnar 0 siblings, 1 reply; 4+ messages in thread From: Stefan Richter @ 2008-10-26 11:02 UTC (permalink / raw) To: linux1394-devel; +Cc: bugme-daemon, linux-kernel, Dan Dennedy, Johannes Weiner Regression in 2.6.28-rc1: When I added the new state_mutex which prevents corruption of raw1394's internal state when accessed by multithreaded client applications, the following possible though highly unlikely deadlock slipped in: Thread A: Thread B: - acquire mmap_sem - raw1394_write() or raw1394_ioctl() - raw1394_mmap() - acquire state_mutex - acquire state_mutex - copy_to/from_user(), possible page fault: acquire mmap_sem The simplest fix is to use mutex_trylock() instead of mutex_lock() in raw1394_mmap(). This changes the behavior under contention in a way which is visible to userspace clients. However, since multithreaded access was entirely buggy before state_mutex was added and libraw1394's documentation advised application programmers to use a handle only in a single thread, this change in behaviour should not be an issue in practice at all. Since we have to use mutex_trylock() in raw1394_mmap() regardless whether /dev/raw1394 was opened with O_NONBLOCK or not, we now use mutex_trylock() unconditionally everywhere for state_mutex, just to have consistent behavior. Reported-by: Johannes Weiner <hannes@saeurebad.de> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- Background: That new state_mutex went only in because raw1394_ioctl() already head some weak protection by the Big Kernel Lock, which I removed for the general reasons pro BKL removal (get better performance with local locks; make the locking clearer, easier to debug, more reliable). drivers/ieee1394/raw1394.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) Index: linux/drivers/ieee1394/raw1394.c =================================================================== --- linux.orig/drivers/ieee1394/raw1394.c +++ linux/drivers/ieee1394/raw1394.c @@ -2268,7 +2268,8 @@ static ssize_t raw1394_write(struct file return -EFAULT; } - mutex_lock(&fi->state_mutex); + if (!mutex_trylock(&fi->state_mutex)) + return -EAGAIN; switch (fi->state) { case opened: @@ -2548,7 +2549,8 @@ static int raw1394_mmap(struct file *fil struct file_info *fi = file->private_data; int ret; - mutex_lock(&fi->state_mutex); + if (!mutex_trylock(&fi->state_mutex)) + return -EAGAIN; if (fi->iso_state == RAW1394_ISO_INACTIVE) ret = -EINVAL; @@ -2669,7 +2671,8 @@ static long raw1394_ioctl(struct file *f break; } - mutex_lock(&fi->state_mutex); + if (!mutex_trylock(&fi->state_mutex)) + return -EAGAIN; switch (fi->iso_state) { case RAW1394_ISO_INACTIVE: -- Stefan Richter -=====-==--- =-=- ==-=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Bug 11824][PATCH] ieee1394: raw1394: fix possible deadlock in multithreaded clients 2008-10-26 11:02 ` [Bug 11824][PATCH] ieee1394: raw1394: fix possible deadlock in multithreaded clients Stefan Richter @ 2008-10-27 10:13 ` Ingo Molnar 2008-10-27 13:37 ` Stefan Richter 0 siblings, 1 reply; 4+ messages in thread From: Ingo Molnar @ 2008-10-27 10:13 UTC (permalink / raw) To: Stefan Richter Cc: linux1394-devel, bugme-daemon, linux-kernel, Dan Dennedy, Johannes Weiner, Peter Zijlstra * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote: > Regression in 2.6.28-rc1: When I added the new state_mutex which > prevents corruption of raw1394's internal state when accessed by > multithreaded client applications, the following possible though > highly unlikely deadlock slipped in: > > Thread A: Thread B: > - acquire mmap_sem - raw1394_write() or raw1394_ioctl() > - raw1394_mmap() - acquire state_mutex > - acquire state_mutex - copy_to/from_user(), possible page fault: > acquire mmap_sem > > The simplest fix is to use mutex_trylock() instead of mutex_lock() in > raw1394_mmap(). This changes the behavior under contention in a way > which is visible to userspace clients. However, since multithreaded > access was entirely buggy before state_mutex was added and libraw1394's > documentation advised application programmers to use a handle only in a > single thread, this change in behaviour should not be an issue in > practice at all. > > Since we have to use mutex_trylock() in raw1394_mmap() regardless > whether /dev/raw1394 was opened with O_NONBLOCK or not, we now use > mutex_trylock() unconditionally everywhere for state_mutex, just to have > consistent behavior. > > Reported-by: Johannes Weiner <hannes@saeurebad.de> > Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> > --- > > Background: That new state_mutex went only in because raw1394_ioctl() > already head some weak protection by the Big Kernel Lock, which I > removed for the general reasons pro BKL removal (get better performance > with local locks; make the locking clearer, easier to debug, more > reliable). > > drivers/ieee1394/raw1394.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > Index: linux/drivers/ieee1394/raw1394.c > =================================================================== > --- linux.orig/drivers/ieee1394/raw1394.c > +++ linux/drivers/ieee1394/raw1394.c > @@ -2268,7 +2268,8 @@ static ssize_t raw1394_write(struct file > return -EFAULT; > } > > - mutex_lock(&fi->state_mutex); > + if (!mutex_trylock(&fi->state_mutex)) > + return -EAGAIN; > > switch (fi->state) { > case opened: > @@ -2548,7 +2549,8 @@ static int raw1394_mmap(struct file *fil > struct file_info *fi = file->private_data; > int ret; > > - mutex_lock(&fi->state_mutex); > + if (!mutex_trylock(&fi->state_mutex)) > + return -EAGAIN; > > if (fi->iso_state == RAW1394_ISO_INACTIVE) > ret = -EINVAL; > @@ -2669,7 +2671,8 @@ static long raw1394_ioctl(struct file *f > break; > } > > - mutex_lock(&fi->state_mutex); > + if (!mutex_trylock(&fi->state_mutex)) > + return -EAGAIN; So we can return a spurious -EAGAIN to user-space, if the state_mutex is taken briefly by some other context? Ingo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Bug 11824][PATCH] ieee1394: raw1394: fix possible deadlock in multithreaded clients 2008-10-27 10:13 ` Ingo Molnar @ 2008-10-27 13:37 ` Stefan Richter 2008-10-27 13:53 ` Stefan Richter 0 siblings, 1 reply; 4+ messages in thread From: Stefan Richter @ 2008-10-27 13:37 UTC (permalink / raw) To: Ingo Molnar Cc: linux1394-devel, bugme-daemon, linux-kernel, Dan Dennedy, Johannes Weiner, Peter Zijlstra Ingo Molnar wrote: > * Stefan Richter <stefanr@s5r6.in-berlin.de> wrote: > >> Regression in 2.6.28-rc1: When I added the new state_mutex which >> prevents corruption of raw1394's internal state when accessed by >> multithreaded client applications, the following possible though >> highly unlikely deadlock slipped in: >> >> Thread A: Thread B: >> - acquire mmap_sem - raw1394_write() or raw1394_ioctl() >> - raw1394_mmap() - acquire state_mutex >> - acquire state_mutex - copy_to/from_user(), possible page fault: >> acquire mmap_sem >> >> The simplest fix is to use mutex_trylock() instead of mutex_lock() in >> raw1394_mmap(). [...] >> Since we have to use mutex_trylock() in raw1394_mmap() regardless >> whether /dev/raw1394 was opened with O_NONBLOCK or not, we now use >> mutex_trylock() unconditionally everywhere for state_mutex, just to have >> consistent behavior. [...] >> Background: That new state_mutex went only in because raw1394_ioctl() >> already head some weak protection by the Big Kernel Lock, which I >> removed for the general reasons pro BKL removal (get better performance >> with local locks; make the locking clearer, easier to debug, more >> reliable). >> >> drivers/ieee1394/raw1394.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> Index: linux/drivers/ieee1394/raw1394.c >> =================================================================== >> --- linux.orig/drivers/ieee1394/raw1394.c >> +++ linux/drivers/ieee1394/raw1394.c >> @@ -2268,7 +2268,8 @@ static ssize_t raw1394_write(struct file >> return -EFAULT; >> } >> >> - mutex_lock(&fi->state_mutex); >> + if (!mutex_trylock(&fi->state_mutex)) >> + return -EAGAIN; >> >> switch (fi->state) { >> case opened: >> @@ -2548,7 +2549,8 @@ static int raw1394_mmap(struct file *fil >> struct file_info *fi = file->private_data; >> int ret; >> >> - mutex_lock(&fi->state_mutex); >> + if (!mutex_trylock(&fi->state_mutex)) >> + return -EAGAIN; >> >> if (fi->iso_state == RAW1394_ISO_INACTIVE) >> ret = -EINVAL; >> @@ -2669,7 +2671,8 @@ static long raw1394_ioctl(struct file *f >> break; >> } >> >> - mutex_lock(&fi->state_mutex); >> + if (!mutex_trylock(&fi->state_mutex)) >> + return -EAGAIN; > > So we can return a spurious -EAGAIN to user-space, if the state_mutex > is taken briefly by some other context? .write() and .mmap() were not serialized against each other and against .ioctl() at all in raw1394 before 2.6.28-rc1. Only .ioctl() was (somewhat) serialized against itself due to traditionally being implemented as .ioctl() instead of .unlocked_ioctl(). If there were ever concurrent userspace contexts accessing the same file handle, they would have corrupted raw1394's internal state eventually. Application developers have been advised all the time to access the file handle only from a single thread, ever. So, before: Concurrent access resulted in hard to debug driver malfunction, without clear error indication. After: Concurrent access results in -EAGAIN. I.e. since concurrent access was so broken before that hopefully no raw1394/libraw1394 client exists which did use a single handle in more than one thread, we are quite free how we fix that. Or at least that's how I understand the situation. Would be good to hear opinions from those who are more involved with libraw1394. -- Stefan Richter -=====-==--- =-=- ==-=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Bug 11824][PATCH] ieee1394: raw1394: fix possible deadlock in multithreaded clients 2008-10-27 13:37 ` Stefan Richter @ 2008-10-27 13:53 ` Stefan Richter 0 siblings, 0 replies; 4+ messages in thread From: Stefan Richter @ 2008-10-27 13:53 UTC (permalink / raw) To: Ingo Molnar Cc: linux1394-devel, bugme-daemon, linux-kernel, Dan Dennedy, Johannes Weiner, Peter Zijlstra I wrote: > .write() and .mmap() were not serialized against each other and against > .ioctl() at all in raw1394 before 2.6.28-rc1. PS: There is a need for serialization to some degree because the client registers itself with a controller via .write() (among many other things that are implemented through .write()), manages isochronous I/O contexts on this controller via .ioctl() and maps DMA buffers for isochronous I/O via .mmap(). The raw1394 driver tracks respective state by means of two state variables and some other variables, and accesses of the state variables is not reentrant within one opener of /dev/raw1394. AFAICS the issue exists between .write() and .write(), and independently of that between .ioctl() and .ioctl() and between .ioctl() and .mmap(). Local mutex protection is the simplest way to fix that --- except that there is this obscure issue of locking order between the driver's mutex and the mmap semaphore outside the driver. -- Stefan Richter -=====-==--- =-=- ==-=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-10-27 13:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <bug-11824-4803@http.bugzilla.kernel.org/>
[not found] ` <4902F41E.5070306@s5r6.in-berlin.de>
2008-10-26 11:02 ` [Bug 11824][PATCH] ieee1394: raw1394: fix possible deadlock in multithreaded clients Stefan Richter
2008-10-27 10:13 ` Ingo Molnar
2008-10-27 13:37 ` Stefan Richter
2008-10-27 13:53 ` Stefan Richter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox