Re: [PATCH] on CONFIG_MM_OWNER=y, kernel panic is possible. take2

From: Balbir Singh
Date: Thu May 08 2008 - 09:54:20 EST


KOSAKI Motohiro wrote:
>> I'd word it as
>>
>> /*
>> * "owner" points to a task that is regarded as the canonical
>> * user/owner of this mm. All of the following must be true in
>> * order for it to be changed:
>> *
>> * current == mm->owner
>> * current->mm != mm
>> * new_owner->mm == mm
>> * new_owner->alloc_lock is held
>> */
>
> Wow, Thank you a lot!
> new version attached.
>
> Cheers!
>
>
> -----------------------------------------------------------
> When mm destruct happend, We should pass mm_update_next_owner()
> old mm.
> but unfortunately new mm is passed in exec_mmap().
>
> thus, kernel panic is possible when multi thread process use exec().
>
>
> and, owner member comment description is wrong.
> mm->owner don't not necessarily point to thread group leader.
>
>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> CC: Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx>
> CC: "Paul Menage" <menage@xxxxxxxxxx>
> CC: "KAMEZAWA Hiroyuki" <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>
> ---
> fs/exec.c | 2 +-
> include/linux/mm_types.h | 13 +++++++++++--
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> Index: b/fs/exec.c
> ===================================================================
> --- a/fs/exec.c 2008-05-04 22:57:09.000000000 +0900
> +++ b/fs/exec.c 2008-05-06 15:40:35.000000000 +0900
> @@ -735,7 +735,7 @@ static int exec_mmap(struct mm_struct *m
> tsk->active_mm = mm;
> activate_mm(active_mm, mm);
> task_unlock(tsk);
> - mm_update_next_owner(mm);
> + mm_update_next_owner(old_mm);
> arch_pick_mmap_layout(mm);
> if (old_mm) {
> up_read(&old_mm->mmap_sem);
> Index: b/include/linux/mm_types.h
> ===================================================================
> --- a/include/linux/mm_types.h 2008-05-08 09:20:13.000000000 +0900
> +++ b/include/linux/mm_types.h 2008-05-08 09:22:13.000000000 +0900
> @@ -231,8 +231,17 @@ struct mm_struct {
> rwlock_t ioctx_list_lock; /* aio lock */
> struct kioctx *ioctx_list;
> #ifdef CONFIG_MM_OWNER
> - struct task_struct *owner; /* The thread group leader that */
> - /* owns the mm_struct. */
> + /*
> + * "owner" points to a task that is regarded as the canonical
> + * user/owner of this mm. All of the following must be true in
> + * order for it to be changed:
> + *
> + * current == mm->owner
> + * current->mm != mm
> + * new_owner->mm == mm
> + * new_owner->alloc_lock is held
> + */
> + struct task_struct *owner;
> #endif
>
> #ifdef CONFIG_PROC_FS
>

Looks good to me, but I've not tested it

Acked-by: Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx>

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/