Skip to contentSkip to conversation

de-semaphorize smackfs

PATCH In review Jonathan Corbet 4 participants 4 messages Mar 17, 2008 – Mar 19, 2008

Subsystems (2)
Security (LSM) Smack security module
lore
Share & tools
lore

Jonathan Corbet

author
#0 lore raw

PATCH 1/1 de-semaphorize smackfs

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 <[email protected]>
security/smack/smackfs.c +4 -4
index afe7c9b..6e09cec 100644
@@ -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);
#1 lore raw
Quoted 6 lines · 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?

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

#2 lore raw

Hi!,

Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · 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?

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

Quoted 4 lines

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

Signature · 3 lines
Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com
#3 lore raw
In reply to Jonathan Corbet

security/smack/smackfs.c static int smk_open_load(struct inode *inode, struct file *file) · line 177

Quoted 9 lines · Jonathan Corbet
security/smack/smackfs.c +1 -1
@@ -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