We met a kernel crash issue when running stress-ng testing, and the
system crashes when printing the dentry name in dump_mapping().
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
pc : dentry_name+0xd8/0x224
lr : pointer+0x22c/0x370
sp : ffff800025f134c0
.....
Call trace:
dentry_name+0xd8/0x224
pointer+0x22c/0x370
vsnprintf+0x1ec/0x730
vscnprintf+0x2c/0x60
vprintk_store+0x70/0x234
vprintk_emit+0xe0/0x24c
vprintk_default+0x3c/0x44
vprintk_func+0x84/0x2d0
printk+0x64/0x88
__dump_page+0x52c/0x530
dump_page+0x14/0x20
set_migratetype_isolate+0x110/0x224
start_isolate_page_range+0xc4/0x20c
offline_pages+0x124/0x474
memory_block_offline+0x44/0xf4
memory_subsys_offline+0x3c/0x70
device_offline+0xf0/0x120
......
The root cause is that, one thread is doing page migration, and we will
use the target page's ->mapping field to save 'anon_vma' pointer between
page unmap and page move, and now the target page is locked and refcount
is 1.
Currently, there is another stress-ng thread performing memory hotplug,
attempting to offline the target page that is being migrated. It discovers
that the refcount of this target page is 1, preventing the offline operation,
thus proceeding to dump the page. However, page_mapping() of the target
page may return an incorrect file mapping to crash the system in dump_mapping(),
since the target page->mapping only saves 'anon_vma' pointer without setting
PAGE_MAPPING_ANON flag.
The page migration issue has been fixed by commit d1adb25df711 ("mm: migrate:
fix getting incorrect page mapping during page migration"). In addition,
Matthew suggested we should also improve dump_mapping()'s robustness to
resilient against the kernel crash [1].
With checking the 'dentry.parent' and 'dentry.d_name.name' used by
dentry_name(), I can see dump_mapping() will output the invalid dentry
instead of crashing the system when this issue is reproduced again.
[12211.189128] page:fffff7de047741c0 refcount:1 mapcount:0 mapping:ffff989117f55ea0 index:0x1 pfn:0x211dd07
[12211.189144] aops:0x0 ino:1 invalid dentry:74786574206e6870
[12211.189148] flags: 0x57ffffc0000001(locked|node=1|zone=2|lastcpupid=0x1fffff)
[12211.189150] page_type: 0xffffffff()
[12211.189153] raw: 0057ffffc0000001 0000000000000000 dead000000000122 ffff989117f55ea0
[12211.189154] raw: 0000000000000001 0000000000000001 00000001ffffffff 0000000000000000
[12211.189155] page dumped because: unmovable page
[1] https://lore.kernel.org/all/ZXxn%[email protected]/
Suggested-by: Matthew Wilcox <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
fs/inode.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/inode.c b/fs/inode.c
index 99d8754a74a3..3093e3b3fd12 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -589,7 +589,8 @@ void dump_mapping(const struct address_space *mapping)
}
dentry_ptr = container_of(dentry_first, struct dentry, d_u.d_alias);
- if (get_kernel_nofault(dentry, dentry_ptr)) {
+ if (get_kernel_nofault(dentry, dentry_ptr) ||
+ !dentry.d_parent || !dentry.d_name.name) {
pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
a_ops, ino, dentry_ptr);
return;
--
2.39.3
On Tue, 16 Jan 2024 15:53:35 +0800, Baolin Wang wrote:
> We met a kernel crash issue when running stress-ng testing, and the
> system crashes when printing the dentry name in dump_mapping().
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> pc : dentry_name+0xd8/0x224
> lr : pointer+0x22c/0x370
> sp : ffff800025f134c0
> ......
> Call trace:
> dentry_name+0xd8/0x224
> pointer+0x22c/0x370
> vsnprintf+0x1ec/0x730
> vscnprintf+0x2c/0x60
> vprintk_store+0x70/0x234
> vprintk_emit+0xe0/0x24c
> vprintk_default+0x3c/0x44
> vprintk_func+0x84/0x2d0
> printk+0x64/0x88
> __dump_page+0x52c/0x530
> dump_page+0x14/0x20
> set_migratetype_isolate+0x110/0x224
> start_isolate_page_range+0xc4/0x20c
> offline_pages+0x124/0x474
> memory_block_offline+0x44/0xf4
> memory_subsys_offline+0x3c/0x70
> device_offline+0xf0/0x120
> ......
>
> [...]
Seems fine for debugging purposes. Let me know if this needs to go through
somewhere else.
---
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] fs: improve dump_mapping() robustness
https://git.kernel.org/vfs/vfs/c/30a1b9d12728
On 1/16/2024 7:16 PM, Christian Brauner wrote:
> On Tue, 16 Jan 2024 15:53:35 +0800, Baolin Wang wrote:
>> We met a kernel crash issue when running stress-ng testing, and the
>> system crashes when printing the dentry name in dump_mapping().
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> pc : dentry_name+0xd8/0x224
>> lr : pointer+0x22c/0x370
>> sp : ffff800025f134c0
>> ......
>> Call trace:
>> dentry_name+0xd8/0x224
>> pointer+0x22c/0x370
>> vsnprintf+0x1ec/0x730
>> vscnprintf+0x2c/0x60
>> vprintk_store+0x70/0x234
>> vprintk_emit+0xe0/0x24c
>> vprintk_default+0x3c/0x44
>> vprintk_func+0x84/0x2d0
>> printk+0x64/0x88
>> __dump_page+0x52c/0x530
>> dump_page+0x14/0x20
>> set_migratetype_isolate+0x110/0x224
>> start_isolate_page_range+0xc4/0x20c
>> offline_pages+0x124/0x474
>> memory_block_offline+0x44/0xf4
>> memory_subsys_offline+0x3c/0x70
>> device_offline+0xf0/0x120
>> ......
>>
>> [...]
>
> Seems fine for debugging purposes. Let me know if this needs to go through
> somewhere else.
Going through VFS tree is fine to me. Thanks.
On 1/18/2024 9:38 AM, Al Viro wrote:
> On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote:
>
>> With checking the 'dentry.parent' and 'dentry.d_name.name' used by
>> dentry_name(), I can see dump_mapping() will output the invalid dentry
>> instead of crashing the system when this issue is reproduced again.
>
>> dentry_ptr = container_of(dentry_first, struct dentry, d_u.d_alias);
>> - if (get_kernel_nofault(dentry, dentry_ptr)) {
>> + if (get_kernel_nofault(dentry, dentry_ptr) ||
>> + !dentry.d_parent || !dentry.d_name.name) {
>> pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
>> a_ops, ino, dentry_ptr);
>> return;
>
> That's nowhere near enough. Your ->d_name.name can bloody well be pointing
> to an external name that gets freed right under you. Legitimately so.
>
> Think what happens if dentry has a long name (longer than would fit into
> the embedded array) and gets renamed name just after you copy it into
> a local variable. Old name will get freed. Yes, freeing is RCU-delayed,
> but I don't see anything that would prevent your thread losing CPU
> and not getting it back until after the sucker's been freed.
Yes, that's possible. And this appears to be a use-after-free issue in
the existing code, which is different from the issue that my patch
addressed.
So how about adding a rcu_read_lock() before copying the dentry to a
local variable in case the old name is freed?
On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote:
> With checking the 'dentry.parent' and 'dentry.d_name.name' used by
> dentry_name(), I can see dump_mapping() will output the invalid dentry
> instead of crashing the system when this issue is reproduced again.
> dentry_ptr = container_of(dentry_first, struct dentry, d_u.d_alias);
> - if (get_kernel_nofault(dentry, dentry_ptr)) {
> + if (get_kernel_nofault(dentry, dentry_ptr) ||
> + !dentry.d_parent || !dentry.d_name.name) {
> pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
> a_ops, ino, dentry_ptr);
> return;
That's nowhere near enough. Your ->d_name.name can bloody well be pointing
to an external name that gets freed right under you. Legitimately so.
Think what happens if dentry has a long name (longer than would fit into
the embedded array) and gets renamed name just after you copy it into
a local variable. Old name will get freed. Yes, freeing is RCU-delayed,
but I don't see anything that would prevent your thread losing CPU
and not getting it back until after the sucker's been freed.
On Thu, Jan 18, 2024 at 01:38:57AM +0000, Al Viro wrote:
> On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote:
>
> > With checking the 'dentry.parent' and 'dentry.d_name.name' used by
> > dentry_name(), I can see dump_mapping() will output the invalid dentry
> > instead of crashing the system when this issue is reproduced again.
>
> > dentry_ptr = container_of(dentry_first, struct dentry, d_u.d_alias);
> > - if (get_kernel_nofault(dentry, dentry_ptr)) {
> > + if (get_kernel_nofault(dentry, dentry_ptr) ||
> > + !dentry.d_parent || !dentry.d_name.name) {
> > pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
> > a_ops, ino, dentry_ptr);
> > return;
>
> That's nowhere near enough. Your ->d_name.name can bloody well be pointing
> to an external name that gets freed right under you. Legitimately so.
>
> Think what happens if dentry has a long name (longer than would fit into
> the embedded array) and gets renamed name just after you copy it into
> a local variable. Old name will get freed. Yes, freeing is RCU-delayed,
> but I don't see anything that would prevent your thread losing CPU
> and not getting it back until after the sucker's been freed.
Agreed that it's not enough. It does usually work, and it's very
helpful when it does. We've had it since 2018 (1c6fb1d89e73) and we've
been gradually making it more robust over time. Part of my reason for
splitting dump_mapping() out of dump_page() was so that it would get
more review from people who understand the fs side of things ... and
that seems to have worked.
Can I trouble you to suggest a more robust solution? Bear in mind that
dump_page() does get called on pointers which turn out not to even be
pointers to struct page so this is all very much best-effort, and giving
up and printing 'this is not a dentry" is always an option.
> Agreed that it's not enough. It does usually work, and it's very
Wasn't the intent to just get somewhat further and accepting that this
might crash no matter what?
Hi Matthew/Baolin,
On 1/18/2024 8:13 AM, Baolin Wang wrote:
>
>
> On 1/18/2024 9:38 AM, Al Viro wrote:
>> On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote:
>>
>>> With checking the 'dentry.parent' and 'dentry.d_name.name' used by
>>> dentry_name(), I can see dump_mapping() will output the invalid dentry
>>> instead of crashing the system when this issue is reproduced again.
>>
>>> dentry_ptr = container_of(dentry_first, struct dentry,
>>> d_u.d_alias);
>>> - if (get_kernel_nofault(dentry, dentry_ptr)) {
>>> + if (get_kernel_nofault(dentry, dentry_ptr) ||
>>> + !dentry.d_parent || !dentry.d_name.name) {
>>> pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
>>> a_ops, ino, dentry_ptr);
>>> return;
>>
>> That's nowhere near enough. Your ->d_name.name can bloody well be
>> pointing
>> to an external name that gets freed right under you. Legitimately so.
>>
>> Think what happens if dentry has a long name (longer than would fit into
>> the embedded array) and gets renamed name just after you copy it into
>> a local variable. Old name will get freed. Yes, freeing is RCU-delayed,
>> but I don't see anything that would prevent your thread losing CPU
>> and not getting it back until after the sucker's been freed.
>
> Yes, that's possible. And this appears to be a use-after-free issue in
> the existing code, which is different from the issue that my patch
> addressed.
>
> So how about adding a rcu_read_lock() before copying the dentry to a
> local variable in case the old name is freed?
>
We too seen the below crash while printing the dentry name.
aops:shmem_aops ino:5e029 dentry name:"dev/zero"
flags:
0x8000000000080006(referenced|uptodate|swapbacked|zone=2|kasantag=0x0)
raw: 8000000000080006 ffffffc033b1bb60 ffffffc033b1bb60 ffffff8862537600
raw: 0000000000000001 0000000000000000 00000003ffffffff ffffff807fe64000
page dumped because: migration failure
migrating pfn aef223 failed ret:1
page:000000009e72a120 refcount:3 mapcount:0 mapping:000000003325dda1
index:0x1 pfn:0xaef223
memcg:ffffff807fe64000
Unable to handle kernel NULL pointer dereference at virtual address
0000000000000000
Mem abort info:
ESR = 0x0000000096000005
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x05: level 1 translation fault
Data abort info:
ISV = 0, ISS = 0x00000005
CM = 0, WnR = 0
user pgtable: 4k pages, 39-bit VAs, pgdp=000000090c12d000
[0000000000000000] pgd=0000000000000000, p4d=0000000000000000,
pud=0000000000000000
Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
dentry_name+0x1f8/0x3a8
pointer+0x3b0/0x6b8
vsnprintf+0x4a4/0x65c
vprintk_store+0x168/0x4a8
vprintk_emit+0x98/0x218
vprintk_default+0x44/0x70
vprintk+0xf0/0x138
_printk+0x54/0x80
dump_mapping+0x17c/0x188
dump_page+0x1d0/0x2e8
offline_pages+0x67c/0x898
Not much comfortable with block layer internals, TMK, the below is what
happening in the my case:
memoffline dput()
(offline_pages) (as part of closing of the shmem file)
------------ --------------------------------------
.......
1) dentry_unlink_inode()
hlist_del_init(&dentry->d_u.d_alias);
2) iput():
a) inode->i_state |= I_FREEING
.....
b) evict_inode()->..->shmem_undo_range
1) get the folios with elevated refcount
3) do_migrate_range():
a) Because of the elevated
refcount in 2.b.1, the
migration of this page will
be failed.
2) truncate_inode_folio() ->
filemap_remove_folio():
(deletes from the page cache,
set page->mapping=NULL,
decrement the refcount on folio)
b) Call dump_page():
1) mapping = page_mapping(page);
2) dump_mapping(mapping)
a) We unlinked the dentry in 1)
thus dentry_ptr from host->i_dentry.first
is not a proper one.
b) dentry name print with %pd is resulting into
the mentioned crash.
At least in this case, I think __this patchset in its current form can
help us__.
Thanks,
Charan
On 1/19/2024 11:48 PM, Charan Teja Kalla wrote:
> Hi Matthew/Baolin,
>
> On 1/18/2024 8:13 AM, Baolin Wang wrote:
>>
>>
>> On 1/18/2024 9:38 AM, Al Viro wrote:
>>> On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote:
>>>
>>>> With checking the 'dentry.parent' and 'dentry.d_name.name' used by
>>>> dentry_name(), I can see dump_mapping() will output the invalid dentry
>>>> instead of crashing the system when this issue is reproduced again.
>>>
>>>> dentry_ptr = container_of(dentry_first, struct dentry,
>>>> d_u.d_alias);
>>>> - if (get_kernel_nofault(dentry, dentry_ptr)) {
>>>> + if (get_kernel_nofault(dentry, dentry_ptr) ||
>>>> + !dentry.d_parent || !dentry.d_name.name) {
>>>> pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
>>>> a_ops, ino, dentry_ptr);
>>>> return;
>>>
>>> That's nowhere near enough. Your ->d_name.name can bloody well be
>>> pointing
>>> to an external name that gets freed right under you. Legitimately so.
>>>
>>> Think what happens if dentry has a long name (longer than would fit into
>>> the embedded array) and gets renamed name just after you copy it into
>>> a local variable. Old name will get freed. Yes, freeing is RCU-delayed,
>>> but I don't see anything that would prevent your thread losing CPU
>>> and not getting it back until after the sucker's been freed.
>>
>> Yes, that's possible. And this appears to be a use-after-free issue in
>> the existing code, which is different from the issue that my patch
>> addressed.
>>
>> So how about adding a rcu_read_lock() before copying the dentry to a
>> local variable in case the old name is freed?
>>
>
> We too seen the below crash while printing the dentry name.
>
> aops:shmem_aops ino:5e029 dentry name:"dev/zero"
> flags:
> 0x8000000000080006(referenced|uptodate|swapbacked|zone=2|kasantag=0x0)
> raw: 8000000000080006 ffffffc033b1bb60 ffffffc033b1bb60 ffffff8862537600
> raw: 0000000000000001 0000000000000000 00000003ffffffff ffffff807fe64000
> page dumped because: migration failure
> migrating pfn aef223 failed ret:1
> page:000000009e72a120 refcount:3 mapcount:0 mapping:000000003325dda1
> index:0x1 pfn:0xaef223
> memcg:ffffff807fe64000
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000000
> Mem abort info:
> ESR = 0x0000000096000005
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x05: level 1 translation fault
> Data abort info:
> ISV = 0, ISS = 0x00000005
> CM = 0, WnR = 0
> user pgtable: 4k pages, 39-bit VAs, pgdp=000000090c12d000
> [0000000000000000] pgd=0000000000000000, p4d=0000000000000000,
> pud=0000000000000000
> Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
>
> dentry_name+0x1f8/0x3a8
> pointer+0x3b0/0x6b8
> vsnprintf+0x4a4/0x65c
> vprintk_store+0x168/0x4a8
> vprintk_emit+0x98/0x218
> vprintk_default+0x44/0x70
> vprintk+0xf0/0x138
> _printk+0x54/0x80
> dump_mapping+0x17c/0x188
> dump_page+0x1d0/0x2e8
> offline_pages+0x67c/0x898
>
>
>
> Not much comfortable with block layer internals, TMK, the below is what
> happening in the my case:
> memoffline dput()
> (offline_pages) (as part of closing of the shmem file)
> ------------ --------------------------------------
> .......
> 1) dentry_unlink_inode()
> hlist_del_init(&dentry->d_u.d_alias);
>
> 2) iput():
> a) inode->i_state |= I_FREEING
> .....
> b) evict_inode()->..->shmem_undo_range
> 1) get the folios with elevated refcount
> 3) do_migrate_range():
> a) Because of the elevated
> refcount in 2.b.1, the
> migration of this page will
> be failed.
>
> 2) truncate_inode_folio() ->
> filemap_remove_folio():
> (deletes from the page cache,
> set page->mapping=NULL,
> decrement the refcount on folio)
> b) Call dump_page():
> 1) mapping = page_mapping(page);
> 2) dump_mapping(mapping)
> a) We unlinked the dentry in 1)
> thus dentry_ptr from host->i_dentry.first
> is not a proper one.
>
> b) dentry name print with %pd is resulting into
> the mentioned crash.
>
>
> At least in this case, I think __this patchset in its current form can
> help us__.
This looks another case of NULL pointer access. Thanks for the detailed
analysis. Could you provide a Tested-by or Reviewed-by tag if it can
solve your problem?
On 1/22/2024 12:47 PM, Baolin Wang wrote:
>>
>> We too seen the below crash while printing the dentry name.
>>
>> aops:shmem_aops ino:5e029 dentry name:"dev/zero"
>> flags:
>> 0x8000000000080006(referenced|uptodate|swapbacked|zone=2|kasantag=0x0)
>> raw: 8000000000080006 ffffffc033b1bb60 ffffffc033b1bb60 ffffff8862537600
>> raw: 0000000000000001 0000000000000000 00000003ffffffff ffffff807fe64000
>> page dumped because: migration failure
>> migrating pfn aef223 failed ret:1
>> page:000000009e72a120 refcount:3 mapcount:0 mapping:000000003325dda1
>> index:0x1 pfn:0xaef223
>> memcg:ffffff807fe64000
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 0000000000000000
>> Mem abort info:
>> ESR = 0x0000000096000005
>> EC = 0x25: DABT (current EL), IL = 32 bits
>> SET = 0, FnV = 0
>> EA = 0, S1PTW = 0
>> FSC = 0x05: level 1 translation fault
>> Data abort info:
>> ISV = 0, ISS = 0x00000005
>> CM = 0, WnR = 0
>> user pgtable: 4k pages, 39-bit VAs, pgdp=000000090c12d000
>> [0000000000000000] pgd=0000000000000000, p4d=0000000000000000,
>> pud=0000000000000000
>> Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
>>
>> dentry_name+0x1f8/0x3a8
>> pointer+0x3b0/0x6b8
>> vsnprintf+0x4a4/0x65c
>> vprintk_store+0x168/0x4a8
>> vprintk_emit+0x98/0x218
>> vprintk_default+0x44/0x70
>> vprintk+0xf0/0x138
>> _printk+0x54/0x80
>> dump_mapping+0x17c/0x188
>> dump_page+0x1d0/0x2e8
>> offline_pages+0x67c/0x898
>>
>>
>>
>> Not much comfortable with block layer internals, TMK, the below is what
>> happening in the my case:
>> memoffline dput()
>> (offline_pages) (as part of closing of the shmem file)
>> ------------ --------------------------------------
>> .......
>> 1) dentry_unlink_inode()
>> hlist_del_init(&dentry->d_u.d_alias);
>>
>> 2) iput():
>> a) inode->i_state |= I_FREEING
>> .....
>> b) evict_inode()->..->shmem_undo_range
>> 1) get the folios with elevated refcount
>> 3) do_migrate_range():
>> a) Because of the elevated
>> refcount in 2.b.1, the
>> migration of this page will
>> be failed.
>>
>> 2) truncate_inode_folio() ->
>> filemap_remove_folio():
>> (deletes from the page cache,
>> set page->mapping=NULL,
>> decrement the refcount on folio)
>> b) Call dump_page():
>> 1) mapping = page_mapping(page);
>> 2) dump_mapping(mapping)
>> a) We unlinked the dentry in 1)
>> thus dentry_ptr from host->i_dentry.first
>> is not a proper one.
>>
>> b) dentry name print with %pd is resulting into
>> the mentioned crash.
>>
>>
>> At least in this case, I think __this patchset in its current form can
>> help us__.
>
> This looks another case of NULL pointer access. Thanks for the detailed
> analysis. Could you provide a Tested-by or Reviewed-by tag if it can
> solve your problem?
Seen this issue couple of times, over 3 months back. Not sure if we ever
encounter this issue again. Still, will pick this and let you know the
side effects of this patch, after thorough testing.
Thanks.