Re: [PATCH][2.4.23-pre7] alternate fix for BUG() in exec_mmap()

[Andrea Arcangeli <andrea@suse.de>]

On Mon, Oct 13, 2003 at 09:11:27PM -0400, Ernie Petrides wrote:
> Yes, this in fact necessary.  I have retested with the locking sequence
> shown above and verified that the problem is still fixed.
> 
> Further, I've discovered that in my original version, with the mmap_sem
> held across mm_release() and exit_aio(), there are potential deadlocks
> in at least the following hypothetical calling trees:
> 
> 	mm_release()
> 	  put_user()
> 	    direct_put_user()
> 	      __put_user_check()
> 	        __put_user_size()
> 	          __put_user_asm()
> 	            [page fault]
> 	                do_page_fault()
> 	                  down_read(&mm->mmap_sem)
> 
> 	exit_aio()
> 	  aio_cancel_all()
> 	    async_poll_cancel()
> 	      aio_put_req()
> 	        put_ioctx()
> 	          __put_ioctx()
> 	            aio_free_ring()
> 	              down_write(&ctx->mm->mmap_sem)
> 
> The corrected patch against 2.4.23-pre7, which restores the fast path in
> exec_mmap() and adds the holding of "mmap_sem" across only the exit_mmap()
> call, is attached below.  Since "mmap_sem" locking is conceptually higher
> than file system locks in the locking hierarchy, the whole calling tree
> from exit_mmap() on down (including potential fput() calls) should be
> safe to run with "mmap_sem" owned.
> 
> Thanks for the help.
> 
> Cheers.  -ernie
> 
> 
> 
> --- linux-2.4.23-pre7/fs/exec.c.orig
> +++ linux-2.4.23-pre7/fs/exec.c
> @@ -426,6 +426,13 @@ static int exec_mmap(void)
>  	struct mm_struct * mm, * old_mm;
>  
>  	old_mm = current->mm;
> +	if (old_mm && atomic_read(&old_mm->mm_users) == 1) {
> +		mm_release();
> +		down_write(&old_mm->mmap_sem);
> +		exit_mmap(old_mm);
> +		up_write(&old_mm->mmap_sem);
> +		return 0;
> +	}
>  
>  	mm = mm_alloc();
>  	if (mm) {

looks fine thanks.

Andrea