[PATCH] de-semaphorize smackfs

[PATCH] de-semaphorize smackfs

[Jonathan Corbet]

While looking at the generic semaphore patch, I came to wonder what the
remaining semaphore users in the kernel were actually doing.  After a
quick grep for down_interruptible(), smackfs remained at the bottom of
my screen.  It seems like a straightforward mutex case - low-hanging
fruit.  So here's a conversion patch; compile-tested only, but what
could go wrong?

jon

Convert smackfs from a semaphore to a mutex

Signed-off-by: Jonathan Corbet <corbet@lwn.net>

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index afe7c9b..6e09cec 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -76,7 +76,7 @@ struct smk_list_entry *smack_list;
 /*
  * Disable concurrent writing open() operations
  */
-static struct semaphore smack_write_sem;
+static DEFINE_MUTEX(smack_write_mutex);
 
 /*
  * Values for parsing cipso rules
@@ -171,7 +171,7 @@ static int smk_open_load(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
 		return seq_open(file, &load_seq_ops);
 
-	if (down_interruptible(&smack_write_sem))
+	if (mutex_lock_interruptible(&smack_write_mutex))
 		return -ERESTARTSYS;
 
 	return 0;
@@ -192,7 +192,7 @@ static int smk_release_load(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
 		return seq_release(inode, file);
 
-	up(&smack_write_sem);
+	mutex_unlock(&smack_write_mutex);
 	return 0;
 }
 
@@ -1011,7 +1011,7 @@ static int __init init_smk_fs(void)
 		}
 	}
 
-	sema_init(&smack_write_sem, 1);
+	mutex_init(&smack_write_mutex);
 	smk_cipso_doi();
 	smk_unlbl_ambient(NULL);
 

Re: [PATCH] de-semaphorize smackfs

[Christoph Hellwig]

On Mon, Mar 17, 2008 at 05:14:40PM -0600, Jonathan Corbet wrote:
> While looking at the generic semaphore patch, I came to wonder what the
> remaining semaphore users in the kernel were actually doing.  After a
> quick grep for down_interruptible(), smackfs remained at the bottom of
> my screen.  It seems like a straightforward mutex case - low-hanging
> fruit.  So here's a conversion patch; compile-tested only, but what
> could go wrong?

The lock is kept while returning to userspace and could potentially
be release by another process.  This is not allowed for mutexes.

Re: [PATCH] de-semaphorize smackfs

[Ahmed S. Darwish]

Hi!,

On Tue, Mar 18, 2008 at 9:35 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Mar 17, 2008 at 05:14:40PM -0600, Jonathan Corbet wrote:
>  > While looking at the generic semaphore patch, I came to wonder what the
>  > remaining semaphore users in the kernel were actually doing.  After a
>  > quick grep for down_interruptible(), smackfs remained at the bottom of
>  > my screen.  It seems like a straightforward mutex case - low-hanging
>  > fruit.  So here's a conversion patch; compile-tested only, but what
>  > could go wrong?
>

I have a feeling that a very nice LWN article is under the way :).

>
>  The lock is kept while returning to userspace and could potentially
>  be release by another process.  This is not allowed for mutexes.
>

Yes, this was an artifact of an old design where different processes
write _sessions_ were not allowed to interleave, thus locking them in
the very beginning (open).

Since now those sessions can be interleaved safely, I'll modify this
issue today to move the locking to each write() call instead.

Thanks Jonathan/Cristoph

-- 
Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH] de-semaphorize smackfs

[Daniel Walker]

On Mon, 2008-03-17 at 17:14 -0600, Jonathan Corbet wrote:

> @@ -171,7 +171,7 @@ static int smk_open_load(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
>  		return seq_open(file, &load_seq_ops);
>  
> -	if (down_interruptible(&smack_write_sem))
> +	if (mutex_lock_interruptible(&smack_write_mutex))
>  		return -ERESTARTSYS;
>  
>  	return 0;

There's a disconnect between the lock unlock calls, when the locking
application returns to userspace. Mutex's have a restriction that you
can't return to userspace while holding the lock.

Daniel