lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Dec 2007 18:06:07 -0800 From: Andrew Morton <akpm@...ux-foundation.org> To: Kevin Winchester <kjwinchester@...il.com> Cc: Ingo Molnar <mingo@...e.hu>, Eric Van Hensbergen <ericvh@...il.com>, Ron Minnich <rminnich@...dia.gov>, Latchesar Ionkov <lucho@...kov.net>, v9fs-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org Subject: Re: [patch 2/2] 9p util semaphore to mutex On Sun, 09 Dec 2007 13:15:11 -0400 Kevin Winchester <kjwinchester@...il.com> wrote: > Convert the semaphore to a mutex in net/9p/util.c > > Signed-off-by: Kevin Winchester <kjwinchester@...il.com> > > --- > net/9p/util.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > Index: v2.6.24-rc4/net/9p/util.c > =================================================================== > --- v2.6.24-rc4.orig/net/9p/util.c > +++ v2.6.24-rc4/net/9p/util.c > @@ -33,7 +33,7 @@ > #include <net/9p/9p.h> > > struct p9_idpool { > - struct semaphore lock; > + struct mutex lock; > struct idr pool; > }; > > @@ -45,7 +45,7 @@ struct p9_idpool *p9_idpool_create(void) > if (!p) > return ERR_PTR(-ENOMEM); > > - init_MUTEX(&p->lock); > + mutex_init(&p->lock); > idr_init(&p->pool); > > return p; > @@ -76,14 +76,14 @@ retry: > if (idr_pre_get(&p->pool, GFP_KERNEL) == 0) > return 0; > > - if (down_interruptible(&p->lock) == -EINTR) { > + if (mutex_lock_interruptible(&p->lock) == -EINTR) { > P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n"); > return -1; > } > > /* no need to store exactly p, we just need something non-null */ > error = idr_get_new(&p->pool, p, &i); > - up(&p->lock); > + mutex_unlock(&p->lock); > > if (error == -EAGAIN) > goto retry; > @@ -104,12 +104,12 @@ EXPORT_SYMBOL(p9_idpool_get); > > void p9_idpool_put(int id, struct p9_idpool *p) > { > - if (down_interruptible(&p->lock) == -EINTR) { > + if (mutex_lock_interruptible(&p->lock) == -EINTR) { > P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n"); > return; > } > idr_remove(&p->pool, id); > - up(&p->lock); > + mutex_unlock(&p->lock); > } > EXPORT_SYMBOL(p9_idpool_put); I cannot see how the code which you are modifying could have been correct. If the lock is contended and this task has signal_pending() then boom - we return from p9_idpool_put() without having removed the item from the IDR tree and then we just lose all track of this fact. It is at a minimum a memory leak. I'm getting the feeling that we need to go and take a general look at the down_interuptible() and mutex_lock_interruptible() callers in the nether regions of the kernel... Meanwhile I'd propose this instead: From: Andrew Morton <akpm@...ux-foundation.org> Don't use down_interruptible() in situations where we cannot handle the consequences. Cc: Kevin Winchester <kjwinchester@...il.com> Cc: Eric Van Hensbergen <ericvh@...il.com> Cc: Ron Minnich <rminnich@...dia.gov> Cc: Latchesar Ionkov <lucho@...kov.net> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org> --- net/9p/util.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff -puN net/9p/util.c~9p-util-fix-semaphore-handling net/9p/util.c --- a/net/9p/util.c~9p-util-fix-semaphore-handling +++ a/net/9p/util.c @@ -76,12 +76,8 @@ retry: if (idr_pre_get(&p->pool, GFP_KERNEL) == 0) return 0; - if (down_interruptible(&p->lock) == -EINTR) { - P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n"); - return -1; - } - /* no need to store exactly p, we just need something non-null */ + down(&p->lock); error = idr_get_new(&p->pool, p, &i); up(&p->lock); @@ -104,10 +100,7 @@ EXPORT_SYMBOL(p9_idpool_get); void p9_idpool_put(int id, struct p9_idpool *p) { - if (down_interruptible(&p->lock) == -EINTR) { - P9_EPRINTK(KERN_WARNING, "Interrupted while locking\n"); - return; - } + down(&p->lock); idr_remove(&p->pool, id); up(&p->lock); } _ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists