[PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[David Howells]

The attached patch adds a missing memory barrier into the key_put() and removes
an unnecessary barrier from install_session_keyring().

install_session_keyring() is also rearranged a little to make it slightly more
efficient.

As install_*_keyring() may schedule (in synchronize_rcu() or keyring_alloc()),
they may not be entered with interrupts disabled - and so there's no point
saving the interrupt disablement state over the critical section.

exec_keys() will also be invoked with interrupts enabled, and so that doesn't
need to save the interrupt state either.
---

 security/keys/key.c          |    1 +
 security/keys/process_keys.c |   41 ++++++++++++++++++++---------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 99781b7..d8a6e00 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -619,6 +619,7 @@ void key_put(struct key *key)
 	if (key) {
 		key_check(key);
 
+		smp_mb__before_atomic_dec();
 		if (atomic_dec_and_test(&key->usage))
 			schedule_work(&key_cleanup_task);
 	}
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 74cb79e..ad123c3 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -167,11 +167,12 @@ error:
  */
 int install_process_keyring(struct task_struct *tsk)
 {
-	unsigned long flags;
 	struct key *keyring;
 	char buf[20];
 	int ret;
 
+	might_sleep();
+
 	if (!tsk->signal->process_keyring) {
 		sprintf(buf, "_pid.%u", tsk->tgid);
 
@@ -182,12 +183,12 @@ int install_process_keyring(struct task_
 		}
 
 		/* attach keyring */
-		spin_lock_irqsave(&tsk->sighand->siglock, flags);
+		spin_lock_irq(&tsk->sighand->siglock);
 		if (!tsk->signal->process_keyring) {
 			tsk->signal->process_keyring = keyring;
 			keyring = NULL;
 		}
-		spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+		spin_unlock_irq(&tsk->sighand->siglock);
 
 		key_put(keyring);
 	}
@@ -206,38 +207,37 @@ error:
 static int install_session_keyring(struct task_struct *tsk,
 				   struct key *keyring)
 {
-	unsigned long flags;
 	struct key *old;
 	char buf[20];
-	int ret;
+
+	might_sleep();
 
 	/* create an empty session keyring */
 	if (!keyring) {
 		sprintf(buf, "_ses.%u", tsk->tgid);
 
 		keyring = keyring_alloc(buf, tsk->uid, tsk->gid, 1, NULL);
-		if (IS_ERR(keyring)) {
-			ret = PTR_ERR(keyring);
-			goto error;
-		}
+		if (IS_ERR(keyring))
+			return PTR_ERR(keyring);
 	}
 	else {
 		atomic_inc(&keyring->usage);
 	}
 
 	/* install the keyring */
-	spin_lock_irqsave(&tsk->sighand->siglock, flags);
-	old = rcu_dereference(tsk->signal->session_keyring);
+	spin_lock_irq(&tsk->sighand->siglock);
+	old = tsk->signal->session_keyring;
 	rcu_assign_pointer(tsk->signal->session_keyring, keyring);
-	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+	spin_unlock_irq(&tsk->sighand->siglock);
 
-	ret = 0;
+	/* we're using RCU on the pointer, but there's no point synchronising
+	 * on it if it didn't previously point to anything */
+	if (old) {
+		synchronize_rcu();
+		key_put(old);
+	}
 
-	/* we're using RCU on the pointer */
-	synchronize_rcu();
-	key_put(old);
-error:
-	return ret;
+	return 0;
 
 } /* end install_session_keyring() */
 
@@ -310,7 +310,6 @@ void exit_keys(struct task_struct *tsk)
  */
 int exec_keys(struct task_struct *tsk)
 {
-	unsigned long flags;
 	struct key *old;
 
 	/* newly exec'd tasks don't get a thread keyring */
@@ -322,10 +321,10 @@ int exec_keys(struct task_struct *tsk)
 	key_put(old);
 
 	/* discard the process keyring from a newly exec'd task */
-	spin_lock_irqsave(&tsk->sighand->siglock, flags);
+	spin_lock_irq(&tsk->sighand->siglock);
 	old = tsk->signal->process_keyring;
 	tsk->signal->process_keyring = NULL;
-	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+	spin_unlock_irq(&tsk->sighand->siglock);
 
 	key_put(old);
 

Re: [Keyrings] [PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> The attached patch adds a missing memory barrier into the key_put() and
> removes an unnecessary barrier from install_session_keyring().
> ...

Signed-Off-By: David Howells <dhowells@redhat.com>

Re: [PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[Nick Piggin]

David Howells wrote:

> diff --git a/security/keys/key.c b/security/keys/key.c
> index 99781b7..d8a6e00 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -619,6 +619,7 @@ void key_put(struct key *key)
>  	if (key) {
>  		key_check(key);
>  
> +		smp_mb__before_atomic_dec();
>  		if (atomic_dec_and_test(&key->usage))
>  			schedule_work(&key_cleanup_task);

Shouldn't be needed: Documentation/atomic_ops.txt specifies that any atomic_
which both modifies its atomic operand and returns something is to be a full
barrier before and after the operation.

This misuse occurs a few times in core code, which makes it a bit confusing.

However, I think it is nice to add a comment if these implicit barriers are
used, if the purpose is not really obvious.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[David Howells]

Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> Shouldn't be needed: Documentation/atomic_ops.txt specifies that any atomic_
> which both modifies its atomic operand and returns something is to be a full
> barrier before and after the operation.

Hmmm... It's possible that I've misunderstood what atomic_ops.txt actually
says.  For instance:

| 	int atomic_inc_and_test(atomic_t *v);
| 	int atomic_dec_and_test(atomic_t *v);
| 
| These two routines increment and decrement by 1, respectively, the
| given atomic counter.  They return a boolean indicating whether the
| resulting counter value was zero or not.
| 
| It requires explicit memory barrier semantics around the operation as
| above.

Note the last paragraph.  "It requires" should be "They require", but the
sense would seem to be obvious.  However, it's not clear on a second reading
as to whether this is an instruction to the _caller_ or an instruction to the
arch _implementer_.

I suppose from reading the abstract at the top:

| This document is intended to serve as a guide to Linux port maintainers on
| how to implement atomic counter, bitops, and spinlock interfaces properly.

that it is meant to be read by the implementor and not the user/caller, in which
case, Nick is correct.

It seems I need to adjust my memory barrier doc, and perhaps I should adjust
atomic_ops.txt too to make it clearer.

David

Re: [PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[Nick Piggin]

David Howells wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> | 	int atomic_inc_and_test(atomic_t *v);
> | 	int atomic_dec_and_test(atomic_t *v);
> | 
> | These two routines increment and decrement by 1, respectively, the
> | given atomic counter.  They return a boolean indicating whether the
> | resulting counter value was zero or not.
> | 
> | It requires explicit memory barrier semantics around the operation as
> | above.
> 
> Note the last paragraph.  "It requires" should be "They require", but the
> sense would seem to be obvious.  However, it's not clear on a second reading
> as to whether this is an instruction to the _caller_ or an instruction to the
> arch _implementer_.
> 

Yes, I remember Dave M clarified this sometime ago (on lkml I guess). It
is a little confusing, but I think the wording is for the implementer's
point of view. Dave will pull me up if I'm wrong...

> I suppose from reading the abstract at the top:
> 
> | This document is intended to serve as a guide to Linux port maintainers on
> | how to implement atomic counter, bitops, and spinlock interfaces properly.
> 
> that it is meant to be read by the implementor and not the user/caller, in which
> case, Nick is correct.
> 
> It seems I need to adjust my memory barrier doc, and perhaps I should adjust
> atomic_ops.txt too to make it clearer.
> 

I think that would be good. atomic_ops.txt is very useful for API users
as well, so if it can be made more general without becoming ambiguous,
I'm sure that would be appreciated.

Thanks,
Nick

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] Keys: Improve usage of memory barriers and remove IRQ disablement

[David S. Miller]

From: Nick Piggin <nickpiggin@yahoo.com.au>
Date: Wed, 05 Apr 2006 19:23:58 +1000

> David Howells wrote:
> > Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> > | 	int atomic_inc_and_test(atomic_t *v);
> > | 	int atomic_dec_and_test(atomic_t *v);
> > | 
> > | These two routines increment and decrement by 1, respectively, the
> > | given atomic counter.  They return a boolean indicating whether the
> > | resulting counter value was zero or not.
> > | 
> > | It requires explicit memory barrier semantics around the operation as
> > | above.
> > 
> > Note the last paragraph.  "It requires" should be "They require", but the
> > sense would seem to be obvious.  However, it's not clear on a second reading
> > as to whether this is an instruction to the _caller_ or an instruction to the
> > arch _implementer_.
> > 
> 
> Yes, I remember Dave M clarified this sometime ago (on lkml I guess). It
> is a little confusing, but I think the wording is for the implementer's
> point of view. Dave will pull me up if I'm wrong...

Any routine which returns state must have the barriers in the arch
implementation.  These two routines returns state.