3rd resend: kill __task_cred()->task_is_dead()
cover 3rd resend: kill __task_cred()->task_is_dead()
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.
PATCH 1/1 creds: kill __task_cred()->task_is_dead() validation
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.
--- include/linux/cred.h | 10 +++------- kernel/exit.c | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-)
include/linux/cred.h +3 -7
@@ -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
kernel/exit.c +1 -1
@@ -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 ))
Signature · 1 line
1.5.5.1
Re: [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
Oleg Nesterov <[email protected]> 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().
Quoted 16 lines
commit
8f92054e:add the following validation condition:
task->exit_state>= 0to 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 usetask_uid()instead. This means we dorcu_read_lock()only to shut up the lockdep, but we already do the same in, say,wait_task_stopped().
Quoted 5 lines
task_is_dead()should die,task->exit_state!= 0 means that this task has passedexit_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 indrivers/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.
Eric
Quoted 44 lines
Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/cred.h | 10 +++------- kernel/exit.c | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-)include/linux/cred.h +3
-7@@ -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 kernel/exit.c +1
-1@@ -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 ))
Re: [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
Quoted 6 lines · Eric W. Biederman
Oleg Nesterov <[email protected]> writes:
A small nit. The subject should be:
"Removetask_is_deadfrom__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.
Quoted 5 lines
Quoted 2 lines
Unfortunately, we can't kill
task_is_dead()right now, it has already found the bugy users indrivers/staging/, the fix already exists.I would say
task_is_dead()has already acquired buggy users indrivers/staging.
Argh, it least I shouldn't have said "bugy".
Quoted 4 lines
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" <[email protected]>
Thanks! I'll fix the subject/changelog and resend with your and David's acks.
Oleg.
Re: [PATCH 1/1] creds: kill __task_cred()->task_is_dead() validation
In reply to Oleg NesterovQuoted 16 lines · Oleg Nesterov
commit
8f92054e:add the following validation condition:
task->exit_state>= 0to 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 usetask_uid()instead. This means we dorcu_read_lock()only to shut up the lockdep, but we already do the same in, say,wait_task_stopped().
Quoted 7 lines
task_is_dead()should die,task->exit_state!= 0 means that this task has passedexit_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 indrivers/staging/, the fix already exists.Signed-off-by Oleg Nesterov <[email protected]>