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.
Signed-off-by: "Yan, Zheng" <[email protected]>
---
mm/memory.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/mm/memory.c b/mm/memory.c
index 5eb3d2524bdc..e51383cd49bf 100644
--- 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;
--
2.13.6
Am 13.12.2017 um 04:58 schrieb Yan, Zheng:
> 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.
>
> Signed-off-by: "Yan, Zheng" <[email protected]>
Reported-and-tested-by: Amon Ott <[email protected]>
Thanks a lot for the patch! I have ported your patch to 4.9.68, tested
and the bug seems fixed now.
Amon Ott
--
Dr. Amon Ott
m-privacy GmbH Tel: +49 30 24342334
Werner-Voß-Damm 62 Fax: +49 30 99296856
12101 Berlin http://www.m-privacy.de
Amtsgericht Charlottenburg, HRB 84946
Geschäftsführer:
Dipl.-Kfm. Holger Maczkowsky,
Roman Maczkowsky
GnuPG-Key-ID: 0x2DD3A649
On Wed, 13 Dec 2017 11:58:36 +0800 "Yan, Zheng" <[email protected]> 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.
> On 14 Dec 2017, at 08:59, Andrew Morton <[email protected]> wrote:
>
> On Wed, 13 Dec 2017 11:58:36 +0800 "Yan, Zheng" <[email protected]> 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.
Background: when there are multiple cephfs clients read/write a file at time same time, read/write should go directly to object store daemon, using page cache is disabled.
ceph_read_iter() uses current->journal_info to pass context information to ceph_readpages(). ceph_readpages() needs to know if its caller has already gotten capability of using page cache (distinguish read from readahead/fadvise). If not, it tries getting the capability by itself. I checked other filesystem, btrfs probably suffers similar problem for its readpages. (verify_parent_transid() uses current->journal_info and it can be called by by btrfs_get_extent())
Regards
Yan, Zheng
>
> 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.
>
>
On Thu, 14 Dec 2017 10:09:58 +0800 "Yan, Zheng" <[email protected]> wrote:
>
>
> > On 14 Dec 2017, at 08:59, Andrew Morton <[email protected]> wrote:
> >
> > On Wed, 13 Dec 2017 11:58:36 +0800 "Yan, Zheng" <[email protected]> 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.
>
>
> Background: when there are multiple cephfs clients read/write a file at time same time, read/write should go directly to object store daemon, using page cache is disabled.
>
> ceph_read_iter() uses current->journal_info to pass context information to ceph_readpages(). ceph_readpages() needs to know if its caller has already gotten capability of using page cache (distinguish read from readahead/fadvise). If not, it tries getting the capability by itself. I checked other filesystem, btrfs probably suffers similar problem for its readpages. (verify_parent_transid() uses current->journal_info and it can be called by by btrfs_get_extent())
>
Ah. Well please let's get all that into the changelog.
> > Can you explain why you chose these two sites? Rather than, for
> > example, way up in handle_mm_fault()?
And please answer this?
> > 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.
> >
> >
>
> On 14 Dec 2017, at 08:59, Andrew Morton <[email protected]> wrote:
>
> On Wed, 13 Dec 2017 11:58:36 +0800 "Yan, Zheng" <[email protected]> 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()?
I think they are the only two places that code can enter another filesystem
>
> 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.
ceph uses current->journal_info for both read/write operations. I think btrfs also read current->journal_info during read-only operation. (I mentioned this in my previous reply)
Regards
Yan, Zheng
On Thu, 14 Dec 2017 10:20:18 +0800 "Yan, Zheng" <[email protected]> wrote:
> >> + /*
> >> + * 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()?
>
> I think they are the only two places that code can enter another filesystem
hm. Maybe. At this point in time. I'm feeling that doing the
save/restore at the highest level is better. It's cheap.
> >
> > 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.
>
> ceph uses current->journal_info for both read/write operations. I think btrfs also read current->journal_info during read-only operation. (I mentioned this in my previous reply)
Quite a lot of filesystems use ->journal_info. Arguably it should be
the fs's responsibility to restore the old journal_info value after
having used it. But that's a ton of changes :(