Re: [PATCH] cred - synchronize rcu before releasing cred

[Jiri Olsa <jolsa@redhat.com>]

On Wed, Jul 28, 2010 at 04:46:27PM +0200, Jiri Olsa wrote:
> On Wed, Jul 28, 2010 at 02:17:27PM +0100, David Howells wrote:
> > 
> > The attached patch should suffice to fix get_task_cred(), and should render
> > Jiri's patch unnecessary.
> > 
> > David
> > ---
> > From: David Howells <dhowells@redhat.com>
> > Subject: [PATCH] CRED: Move get_task_cred() out of line and make it use atomic_inc_not_zero()
> > 
> > It's possible for get_task_cred() as it currently stands to 'corrupt' a set of
> > credentials by incrementing their usage count after their replacement by the
> > task being accessed.
> > 
> > What happens is that get_task_cred() engages the RCU read lock, accesses the cred
> > 
> > 
> > 	TASK_1			TASK_2			RCU_CLEANER
> > 	-->get_task_cred(TASK_2)
> > 	rcu_read_lock()
> > 	__cred = __task_cred(TASK_2)
> > 				-->commit_creds()
> > 				old_cred = TASK_2->real_cred
> > 				TASK_2->real_cred = ...
> > 				put_cred(old_cred)
> > 				  call_rcu(old_cred)
> > 		[__cred->usage == 0]
> > 	get_cred(__cred)
> > 		[__cred->usage == 1]
> > 	rcu_read_unlock()
> > 							-->put_cred_rcu()
> > 							[__cred->usage == 1]
> > 							panic()
> > 
> > However, since a tasks credentials are generally not changed very often, we can
> > reasonably make use of a loop involving reading the creds pointer and using
> > atomic_inc_not_zero() to attempt to increment it if it hasn't already hit zero.
> > 
> > If successful, we can safely return the credentials in the knowledge that, even
> > if the task we're accessing has released them, they haven't gone to the RCU
> > cleanup code.
> 
> looks ok, I changed the task_state to use this and I'm running the
> bug reproducer... so far so good ;)

the test did not hit the issue, so we are good probably

thanks,
jirka