Re: [PATCH] mm: save current->journal_info before calling fault/page_mkwrite

[Andrew Morton]

On Wed, 13 Dec 2017 11:58:36 +0800 "Yan, Zheng" <zyan@xxxxxxxxxx> wrote:

> We recently got an Oops report:
> 
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: jbd2__journal_start+0x38/0x1a2
> [...]
> Call Trace:
>   ext4_page_mkwrite+0x307/0x52b
>   _ext4_get_block+0xd8/0xd8
>   do_page_mkwrite+0x6e/0xd8
>   handle_mm_fault+0x686/0xf9b
>   mntput_no_expire+0x1f/0x21e
>   __do_page_fault+0x21d/0x465
>   dput+0x4a/0x2f7
>   page_fault+0x22/0x30
>   copy_user_generic_string+0x2c/0x40
>   copy_page_to_iter+0x8c/0x2b8
>   generic_file_read_iter+0x26e/0x845
>   timerqueue_del+0x31/0x90
>   ceph_read_iter+0x697/0xa33 [ceph]
>   hrtimer_cancel+0x23/0x41
>   futex_wait+0x1c8/0x24d
>   get_futex_key+0x32c/0x39a
>   __vfs_read+0xe0/0x130
>   vfs_read.part.1+0x6c/0x123
>   handle_mm_fault+0x831/0xf9b
>   __fget+0x7e/0xbf
>   SyS_read+0x4d/0xb5
> 
> The reason is that page fault can happen when one filesystem copies
> data from/to userspace, the filesystem may set current->journal_info.
> If the userspace memory is mapped to a file on another filesystem,
> the later filesystem may also want to use current->journal_info.
> 

whoops.

A cc:stable will be needed here...

A filesystem doesn't "copy data from/to userspace".  I assume here
we're referring to a read() where the source is a pagecache page for
filesystem A and the destination is a MAP_SHARED page in filesystem B?

But in that case I don't see why filesystem A would have a live
->journal_info?  It's just doing a read.

So can we please have more detailed info on the exact scenario here?

> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2347,12 +2347,22 @@ static int do_page_mkwrite(struct vm_fault *vmf)
>  {
>  	int ret;
>  	struct page *page = vmf->page;
> +	void *old_journal_info = current->journal_info;
>  	unsigned int old_flags = vmf->flags;
>  
> +	/*
> +	 * If the fault happens during read_iter() copies data to
> +	 * userspace, filesystem may have set current->journal_info.
> +	 * If the userspace memory is mapped to a file on another
> +	 * filesystem, page_mkwrite() of the later filesystem may
> +	 * want to access/modify current->journal_info.
> +	 */
> +	current->journal_info = NULL;
>  	vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
>  
>  	ret = vmf->vma->vm_ops->page_mkwrite(vmf);
> -	/* Restore original flags so that caller is not surprised */
> +	/* Restore original journal_info and flags */
> +	current->journal_info = old_journal_info;
>  	vmf->flags = old_flags;
>  	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
>  		return ret;
> @@ -3191,9 +3201,20 @@ static int do_anonymous_page(struct vm_fault *vmf)
>  static int __do_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
> +	void *old_journal_info = current->journal_info;
>  	int ret;
>  
> +	/*
> +	 * If the fault happens during write_iter() copies data from
> +	 * userspace, filesystem may have set current->journal_info.
> +	 * If the userspace memory is mapped to a file on another
> +	 * filesystem, fault handler of the later filesystem may want
> +	 * to access/modify current->journal_info.
> +	 */
> +	current->journal_info = NULL;
>  	ret = vma->vm_ops->fault(vmf);
> +	/* Restore original journal_info */
> +	current->journal_info = old_journal_info;
>  	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY |
>  			    VM_FAULT_DONE_COW)))
>  		return ret;

Can you explain why you chose these two sites?  Rather than, for
example, way up in handle_mm_fault()?

It's hard to believe that a fault handler will alter ->journal_info if
it is handling a read fault, so perhaps we only need to do this for a
write fault?  Although such an optimization probably isn't worthwhile. 
The whole thing is only about three instructions.