* [PATCH 0/1] 3rd resend: kill __task_cred()->task_is_dead()
@ 2012-04-10 19:45 Oleg Nesterov
2012-04-10 19:46 ` [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation Oleg Nesterov
2012-04-11 10:41 ` David Howells
0 siblings, 2 replies; 5+ messages in thread
From: Oleg Nesterov @ 2012-04-10 19:45 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Howells, Paul E. McKenney, linux-kernel
Hello.
The patch was acked by David on 2011-09-20, see
http://marc.info/?l=linux-kernel&m=131653612415506
I didn't dare to keep this ack because the change in cred.h
was updated.
task_is_dead() must die ;) Another user, usbip, was already
fixed in the staging git.
Oleg.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
2012-04-10 19:45 [PATCH 0/1] 3rd resend: kill __task_cred()->task_is_dead() Oleg Nesterov
@ 2012-04-10 19:46 ` Oleg Nesterov
2012-04-11 10:09 ` Eric W. Biederman
2012-04-11 10:41 ` David Howells
1 sibling, 1 reply; 5+ messages in thread
From: Oleg Nesterov @ 2012-04-10 19:46 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Howells, Paul E. McKenney, linux-kernel
commit 8f92054e:
add the following validation condition:
task->exit_state >= 0
to permit the access if the target task is dead and therefore
unable to change its own credentials.
OK, but afaics currently this can only help wait_task_zombie() which
calls __task_cred() without rcu lock.
Remove this validation and change wait_task_zombie() to use task_uid()
instead. This means we do rcu_read_lock() only to shut up the lockdep,
but we already do the same in, say, wait_task_stopped().
task_is_dead() should die, task->exit_state != 0 means that this task
has passed exit_notify(), only do_wait-like code paths should use this.
Unfortunately, we can't kill task_is_dead() right now, it has already
found the bugy users in drivers/staging/, the fix already exists.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
include/linux/cred.h | 10 +++-------
kernel/exit.c | 2 +-
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index adadf71..1b64c72 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -276,17 +276,13 @@ static inline void put_cred(const struct cred *_cred)
* @task: The task to query
*
* Access the objective credentials of a task. The caller must hold the RCU
- * readlock or the task must be dead and unable to change its own credentials.
+ * readlock.
*
* The result of this function should not be passed directly to get_cred();
* rather get_task_cred() should be used instead.
*/
-#define __task_cred(task) \
- ({ \
- const struct task_struct *__t = (task); \
- rcu_dereference_check(__t->real_cred, \
- task_is_dead(__t)); \
- })
+#define __task_cred(task) \
+ rcu_dereference((task)->real_cred)
/**
* get_current_cred - Get the current task's subjective credentials
diff --git a/kernel/exit.c b/kernel/exit.c
index 4b4042f..7b36288 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1214,7 +1213,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
unsigned long state;
int retval, status, traced;
pid_t pid = task_pid_vnr(p);
- uid_t uid = __task_cred(p)->uid;
+ uid_t uid = task_uid(p);
struct siginfo __user *infop;
if (!likely(wo->wo_flags & WEXITED))
--
1.5.5.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
2012-04-10 19:46 ` [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation Oleg Nesterov
@ 2012-04-11 10:09 ` Eric W. Biederman
2012-04-11 18:52 ` Oleg Nesterov
0 siblings, 1 reply; 5+ messages in thread
From: Eric W. Biederman @ 2012-04-11 10:09 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, David Howells, Paul E. McKenney, linux-kernel
Oleg Nesterov <oleg@redhat.com> writes:
A small nit. The subject should be:
"Remove task_is_dead from __task_cred() validation."
there is no method __task_cred()->task_is_dead().
> commit 8f92054e:
>
> add the following validation condition:
>
> task->exit_state >= 0
>
> to permit the access if the target task is dead and therefore
> unable to change its own credentials.
>
> OK, but afaics currently this can only help wait_task_zombie() which
> calls __task_cred() without rcu lock.
>
> Remove this validation and change wait_task_zombie() to use task_uid()
> instead. This means we do rcu_read_lock() only to shut up the lockdep,
> but we already do the same in, say, wait_task_stopped().
>
> task_is_dead() should die, task->exit_state != 0 means that this task
> has passed exit_notify(), only do_wait-like code paths should use this.
>
> Unfortunately, we can't kill task_is_dead() right now, it has already
> found the bugy users in drivers/staging/, the fix already exists.
I would say task_is_dead() has already acquired buggy users in
drivers/staging.
As for the patch itself, and the direction of removing task_is_dead().
It looks good from where I sit.
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
Eric
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> include/linux/cred.h | 10 +++-------
> kernel/exit.c | 2 +-
> 2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index adadf71..1b64c72 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -276,17 +276,13 @@ static inline void put_cred(const struct cred *_cred)
> * @task: The task to query
> *
> * Access the objective credentials of a task. The caller must hold the RCU
> - * readlock or the task must be dead and unable to change its own credentials.
> + * readlock.
> *
> * The result of this function should not be passed directly to get_cred();
> * rather get_task_cred() should be used instead.
> */
> -#define __task_cred(task) \
> - ({ \
> - const struct task_struct *__t = (task); \
> - rcu_dereference_check(__t->real_cred, \
> - task_is_dead(__t)); \
> - })
> +#define __task_cred(task) \
> + rcu_dereference((task)->real_cred)
>
> /**
> * get_current_cred - Get the current task's subjective credentials
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 4b4042f..7b36288 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1214,7 +1213,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
> unsigned long state;
> int retval, status, traced;
> pid_t pid = task_pid_vnr(p);
> - uid_t uid = __task_cred(p)->uid;
> + uid_t uid = task_uid(p);
> struct siginfo __user *infop;
>
> if (!likely(wo->wo_flags & WEXITED))
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
2012-04-10 19:45 [PATCH 0/1] 3rd resend: kill __task_cred()->task_is_dead() Oleg Nesterov
2012-04-10 19:46 ` [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation Oleg Nesterov
@ 2012-04-11 10:41 ` David Howells
1 sibling, 0 replies; 5+ messages in thread
From: David Howells @ 2012-04-11 10:41 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: dhowells, Andrew Morton, Paul E. McKenney, linux-kernel
Oleg Nesterov <oleg@redhat.com> wrote:
> commit 8f92054e:
>
> add the following validation condition:
>
> task->exit_state >= 0
>
> to permit the access if the target task is dead and therefore
> unable to change its own credentials.
>
> OK, but afaics currently this can only help wait_task_zombie() which
> calls __task_cred() without rcu lock.
>
> Remove this validation and change wait_task_zombie() to use task_uid()
> instead. This means we do rcu_read_lock() only to shut up the lockdep,
> but we already do the same in, say, wait_task_stopped().
>
> task_is_dead() should die, task->exit_state != 0 means that this task
> has passed exit_notify(), only do_wait-like code paths should use this.
>
> Unfortunately, we can't kill task_is_dead() right now, it has already
> found the bugy users in drivers/staging/, the fix already exists.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
2012-04-11 10:09 ` Eric W. Biederman
@ 2012-04-11 18:52 ` Oleg Nesterov
0 siblings, 0 replies; 5+ messages in thread
From: Oleg Nesterov @ 2012-04-11 18:52 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Andrew Morton, David Howells, Paul E. McKenney, linux-kernel
On 04/11, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> A small nit. The subject should be:
> "Remove task_is_dead from __task_cred() validation."
> there is no method __task_cred()->task_is_dead().
I often use this notation to show the caller and the callee,
but I don't really mind.
> > Unfortunately, we can't kill task_is_dead() right now, it has already
> > found the bugy users in drivers/staging/, the fix already exists.
>
> I would say task_is_dead() has already acquired buggy users in
> drivers/staging.
Argh, it least I shouldn't have said "bugy".
> As for the patch itself, and the direction of removing task_is_dead().
> It looks good from where I sit.
>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
Thanks! I'll fix the subject/changelog and resend with your and
David's acks.
Oleg.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-11 18:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-10 19:45 [PATCH 0/1] 3rd resend: kill __task_cred()->task_is_dead() Oleg Nesterov
2012-04-10 19:46 ` [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation Oleg Nesterov
2012-04-11 10:09 ` Eric W. Biederman
2012-04-11 18:52 ` Oleg Nesterov
2012-04-11 10:41 ` David Howells
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.