mm/filemap: Adding missing mem_cgroup_uncharge() to __add_to_page_cache_locked()

PATCH Applied Waiman Long 8 participants 26 messages Jan 25, 2021 – Jan 26, 2021

Subsystems (2)

Memory management Page cache

lore

Share & tools

lore

Waiman Long

author

2021-01-25 04:25 UTC #0 lore raw

PATCH 1/1 mm/filemap: Adding missing mem_cgroup_uncharge() to __add_to_page_cache_locked()

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

 [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
 [ 1570.068333] pages's memcg:ffff8889a4116000
 [ 1570.068343] ------------[ cut here ]------------
 [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
 [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
 [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
 [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
 [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
   :
 [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
 [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
 [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
 [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
 [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
 [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
 [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
 [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
 [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 [ 1570.068402] PKRU: 55555554
 [ 1570.068404] Call Trace:
 [ 1570.068407]  mem_cgroup_charge+0x175/0x770
 [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
 [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
 [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
 [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
 [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
 [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
 [ 1570.068693]  read_pages+0x5b1/0xc40
 [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
 [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
 [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
 [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
 [ 1570.068872]  new_sync_read+0x3af/0x610
 [ 1570.068901]  vfs_read+0x339/0x4b0
 [ 1570.068909]  ksys_read+0xf1/0x1c0
 [ 1570.068920]  do_syscall_64+0x33/0x40
 [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 [ 1570.068930] RIP: 0033:0x7ff039135595

Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")

Signed-off-by Waiman Long <[email protected]>

---
 mm/filemap.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/filemap.c +4 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
 	int error;
+	bool charged = false;
 
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageSwapBacked(page), page);

@@ -848,6 +849,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 		error = mem_cgroup_charge(page, current->mm, gfp);
 		if (error)
 			goto error;
+		charged = true;
 	}
 
 	gfp &= GFP_RECLAIM_MASK;

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
 		goto error;
 	}

Signature · 1 line

2.18.1

Matthew Wilcox

maintainer

2021-01-25 04:37 UTC #1 lore raw

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 838

Quoted 9 lines · Waiman Long

mm/filemap.c +1 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
 	int error;
+	bool charged = false;

I don't think we need this extra bool.

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 904

Quoted 8 lines

mm/filemap.c +2 -0

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
 		goto error;
 	}

Better:

- goto error;
+ goto uncharge;
...
+uncharge:
+ if (!huge)
+ mem_cgroup_uncharge(page);
error:
...

Miaohe Lin

2021-01-25 06:34 UTC #2 lore raw

Hi:

Quoted 24 lines · Matthew Wilcox

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 838
Quoted 9 lines · Waiman Long
mm/filemap.c +1 -0
index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
 	int error;
+	bool charged = false;
I don't think we need this extra bool.
mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 904
Quoted 8 lines
mm/filemap.c +2 -0
@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
 		goto error;
 	}
Better:

Review

Quoted 5 lines

(quoted diff) +3 -1

-		goto error;
+		goto uncharge;
...
+uncharge:
+	if (!huge)

Since commit 7a02fa97b897 ("secretmem: add memcg accounting"), this may be:

	if (!huge && !page_is_secretmem(page))

Or just bool charged is better?
Many thanks.

Quoted 6 lines

(quoted diff) +1 -0

+		mem_cgroup_uncharge(page);
 error:
...

Waiman Long

author

2021-01-25 14:09 UTC #3 lore raw

In reply to Matthew Wilcox

Quoted 23 lines · Matthew Wilcox

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 838
Quoted 9 lines · Waiman Long
mm/filemap.c +1 -0
index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
  	XA_STATE(xas, &mapping->i_pages, offset);
  	int huge = PageHuge(page);
  	int error;
+	bool charged = false;
I don't think we need this extra bool.
mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 904
Quoted 8 lines
mm/filemap.c +2 -0
@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
  	if (xas_error(&xas)) {
  		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
  		goto error;
  	}
Better:

Review

Quoted 8 lines

(quoted diff) +4 -1

-		goto error;
+		goto uncharge;
...
+uncharge:
+	if (!huge)
+		mem_cgroup_uncharge(page);
  error:
...

That was my original plan. After finding out there was a potentially conflicting patch in linux-next:

commit 7a02fa97b897 ("secretmem: add memcg accounting")

@@ -839,7 +840,7 @@ noinline int __add_to_page_cache_locked(struct page *page, page->mapping = mapping;
page->index = offset;

-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp); if (error)
                        goto error;

Adding a boolean is an easy way out without conflicting it.

Cheers,
Longman

Alex Shi

Reviewed-by on v1 1/1

2021-01-25 04:41 UTC #4 lore raw

In reply to Waiman Long

Reviewed-by Alex Shi <[email protected]>

Quoted 88 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

 [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
 [ 1570.068333] pages's memcg:ffff8889a4116000
 [ 1570.068343] ------------[ cut here ]------------
 [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
 [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
 [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
 [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
 [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
   :
 [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
 [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
 [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
 [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
 [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
 [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
 [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
 [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
 [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 [ 1570.068402] PKRU: 55555554
 [ 1570.068404] Call Trace:
 [ 1570.068407]  mem_cgroup_charge+0x175/0x770
 [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
 [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
 [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
 [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
 [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
 [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
 [ 1570.068693]  read_pages+0x5b1/0xc40
 [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
 [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
 [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
 [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
 [ 1570.068872]  new_sync_read+0x3af/0x610
 [ 1570.068901]  vfs_read+0x339/0x4b0
 [ 1570.068909]  ksys_read+0xf1/0x1c0
 [ 1570.068920]  do_syscall_64+0x33/0x40
 [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 [ 1570.068930] RIP: 0033:0x7ff039135595

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")

Signed-off-by Waiman Long <[email protected]>

---
 mm/filemap.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/filemap.c +4 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
 	int error;
+	bool charged = false;
 
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageSwapBacked(page), page);

@@ -848,6 +849,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 		error = mem_cgroup_charge(page, current->mm, gfp);
 		if (error)
 			goto error;
+		charged = true;
 	}
 
 	gfp &= GFP_RECLAIM_MASK;

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
 		goto error;
 	}

Miaohe Lin

Reviewed-by on v1 1/1

2021-01-25 06:30 UTC #5 lore raw

In reply to Waiman Long

Quoted 52 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

 [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
 [ 1570.068333] pages's memcg:ffff8889a4116000
 [ 1570.068343] ------------[ cut here ]------------
 [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
 [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
 [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
 [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
 [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
   :
 [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
 [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
 [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
 [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
 [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
 [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
 [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
 [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
 [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 [ 1570.068402] PKRU: 55555554
 [ 1570.068404] Call Trace:
 [ 1570.068407]  mem_cgroup_charge+0x175/0x770
 [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
 [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
 [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
 [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
 [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
 [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
 [ 1570.068693]  read_pages+0x5b1/0xc40
 [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
 [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
 [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
 [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
 [ 1570.068872]  new_sync_read+0x3af/0x610
 [ 1570.068901]  vfs_read+0x339/0x4b0
 [ 1570.068909]  ksys_read+0xf1/0x1c0
 [ 1570.068920]  do_syscall_64+0x33/0x40
 [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 [ 1570.068930] RIP: 0033:0x7ff039135595

Quoted 2 lines

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>

Cc stable should be needed.

Quoted 26 lines

---
 mm/filemap.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/filemap.c +2 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
 	int error;
+	bool charged = false;
 
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageSwapBacked(page), page);

@@ -848,6 +849,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 		error = mem_cgroup_charge(page, current->mm, gfp);
 		if (error)
 			goto error;
+		charged = true;
 	}
 
 	gfp &= GFP_RECLAIM_MASK;

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 905

Quoted 8 lines

mm/filemap.c +2 -0

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
 		goto error;
 	}

Looks good to me. Thanks.

Reviewed-by Miaohe Lin <[email protected]>

Waiman Long

author

2021-01-25 14:13 UTC #6 lore raw

Quoted 53 lines · Miaohe Lin

Quoted 52 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595

Quoted 3 lines

Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
Cc stable should be needed.

Yes, this patch should go to stable. I think the stable tree maintainers will automatically pick up patches with the "Fixes" tag. That is why I don't explicitly put a "cc:stable" line in the patch.

Thanks,
Longman

Andrew Morton

maintainer

2021-01-25 23:11 UTC #7 lore raw

Quoted 3 lines · On Mon, 25 Jan 2021 09:12:56 -0500 Waiman Long <[email protected]> wrote:

Yes, this patch should go to stable. I think the stable tree maintainers will automatically pick up patches with the "Fixes" tag. That is why I don't explicitly put a "cc:stable" line in the patch.

No. Both the Fixes: and the cc:stable are needed for the backport. Because sometimes we fix things but don't consider the fix important enough to risk the backport.

Waiman Long

author

2021-01-25 23:13 UTC #8 lore raw

Quoted 9 lines · Andrew Morton

Quoted 3 lines · On Mon, 25 Jan 2021 09:12:56 -0500 Waiman Long <[email protected]> wrote:
Yes, this patch should go to stable. I think the stable tree maintainers will automatically pick up patches with the "Fixes" tag. That is why I don't explicitly put a "cc:stable" line in the patch.
No. Both the Fixes: and the cc:stable are needed for the backport. Because sometimes we fix things but don't consider the fix important enough to risk the backport.

I see. Thanks for the clarification. I will keep that in mind next time.

Cheers,
Longman

Muchun Song

2021-01-25 08:08 UTC #9 lore raw

In reply to Waiman Long

Quoted 80 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

 [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
 [ 1570.068333] pages's memcg:ffff8889a4116000
 [ 1570.068343] ------------[ cut here ]------------
 [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
 [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
 [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
 [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
 [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
   :
 [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
 [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
 [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
 [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
 [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
 [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
 [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
 [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
 [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 [ 1570.068402] PKRU: 55555554
 [ 1570.068404] Call Trace:
 [ 1570.068407]  mem_cgroup_charge+0x175/0x770
 [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
 [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
 [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
 [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
 [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
 [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
 [ 1570.068693]  read_pages+0x5b1/0xc40
 [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
 [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
 [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
 [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
 [ 1570.068872]  new_sync_read+0x3af/0x610
 [ 1570.068901]  vfs_read+0x339/0x4b0
 [ 1570.068909]  ksys_read+0xf1/0x1c0
 [ 1570.068920]  do_syscall_64+0x33/0x40
 [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 [ 1570.068930] RIP: 0033:0x7ff039135595

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")

Signed-off-by Waiman Long <[email protected]>

---
 mm/filemap.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/filemap.c +2 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
        XA_STATE(xas, &mapping->i_pages, offset);
        int huge = PageHuge(page);
        int error;
+       bool charged = false;
 
        VM_BUG_ON_PAGE(!PageLocked(page), page);
        VM_BUG_ON_PAGE(PageSwapBacked(page), page);

@@ -848,6 +849,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
                error = mem_cgroup_charge(page, current->mm, gfp);
                if (error)
                        goto error;
+               charged = true;
        }
 
        gfp &= GFP_RECLAIM_MASK;

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 901

Quoted 4 lines

mm/filemap.c +1 -0

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
        if (xas_error(&xas)) {
                error = xas_error(&xas);
+               if (charged)

Can "if (!huge)" replace "if (charged)"?

Quoted 7 lines

mm/filemap.c +1 -1

+                       mem_cgroup_uncharge(page);
                goto error;
        }
 
--

2.18.1

Waiman Long

author

2021-01-25 15:35 UTC #10 lore raw

Quoted 78 lines · Muchun Song

Quoted 77 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")

Signed-off-by Waiman Long <[email protected]>

---
  mm/filemap.c | 4 ++++
  1 file changed, 4 insertions(+)

mm/filemap.c +2 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
         XA_STATE(xas, &mapping->i_pages, offset);
         int huge = PageHuge(page);
         int error;
+       bool charged = false;
 
         VM_BUG_ON_PAGE(!PageLocked(page), page);
         VM_BUG_ON_PAGE(PageSwapBacked(page), page);

@@ -848,6 +849,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
                 error = mem_cgroup_charge(page, current->mm, gfp);
                 if (error)
                         goto error;
+               charged = true;
         }

Quoted 7 lines

mm/filemap.c noinline int __add_to_page_cache_locked(struct page *page, · line 901

Quoted 6 lines

mm/filemap.c +1 -0

         gfp &= GFP_RECLAIM_MASK;

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
         if (xas_error(&xas)) {
                 error = xas_error(&xas);
+               if (charged)

Can "if (!huge)" replace "if (charged)"?

See my reply to Mathew Wilcox.

Cheers,
Longman

Michal Hocko

2021-01-25 09:28 UTC #11 lore raw

In reply to Waiman Long

Quoted 52 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

 [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
 [ 1570.068333] pages's memcg:ffff8889a4116000
 [ 1570.068343] ------------[ cut here ]------------
 [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
 [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
 [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
 [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
 [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
   :
 [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
 [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
 [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
 [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
 [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
 [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
 [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
 [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
 [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 [ 1570.068402] PKRU: 55555554
 [ 1570.068404] Call Trace:
 [ 1570.068407]  mem_cgroup_charge+0x175/0x770
 [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
 [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
 [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
 [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
 [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
 [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
 [ 1570.068693]  read_pages+0x5b1/0xc40
 [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
 [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
 [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
 [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
 [ 1570.068872]  new_sync_read+0x3af/0x610
 [ 1570.068901]  vfs_read+0x339/0x4b0
 [ 1570.068909]  ksys_read+0xf1/0x1c0
 [ 1570.068920]  do_syscall_64+0x33/0x40
 [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 [ 1570.068930] RIP: 0033:0x7ff039135595

Quoted 2 lines

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>

OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.

I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?

Quoted 36 lines

---
 mm/filemap.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/filemap.c +4 -0

index 5c9d564317a5..aa0e0fb04670 100644
@@ -835,6 +835,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
 	int error;
+	bool charged = false;
 
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageSwapBacked(page), page);

@@ -848,6 +849,7 @@ noinline int __add_to_page_cache_locked(struct page *page,
 		error = mem_cgroup_charge(page, current->mm, gfp);
 		if (error)
 			goto error;
+		charged = true;
 	}
 
 	gfp &= GFP_RECLAIM_MASK;

@@ -896,6 +898,8 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
+		if (charged)
+			mem_cgroup_uncharge(page);
 		goto error;
 	}

Signature · 1 line

2.18.1

Signature · 2 lines

Michal Hocko
SUSE Labs

Waiman Long

author

2021-01-25 15:58 UTC #12 lore raw

Quoted 61 lines · Michal Hocko

Quoted 52 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595

Quoted 2 lines

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>

Quoted 5 lines

I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?

There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.

Cheers,
Longman

Michal Hocko

2021-01-25 16:06 UTC #13 lore raw

Quoted 68 lines · Waiman Long

Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?

Quoted 3 lines

There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.

I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?

Signature · 2 lines

Michal Hocko
SUSE Labs

Matthew Wilcox

maintainer

2021-01-25 16:26 UTC #14 lore raw

Quoted 69 lines · Michal Hocko

Quoted 68 lines · Waiman Long
Quoted 66 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?

Quoted 8 lines

Quoted 3 lines
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?

I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.

I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.

Michal Hocko

2021-01-25 16:41 UTC #15 lore raw

Quoted 74 lines · Matthew Wilcox

Quoted 73 lines · Michal Hocko
Quoted 71 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.

Quoted 7 lines

Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.

Yes, I really had fscache in mind because it does have an "unusual" page life time rules.

Quoted 2 lines

I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.

Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.

But as I've said I am not really familiar with fscache so this might be just unfeasible and a better fix would be in a library code.

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

Acked-by

2021-01-25 18:14 UTC #16 lore raw

Quoted 89 lines · Michal Hocko

Quoted 74 lines · Matthew Wilcox
Quoted 73 lines · Michal Hocko
Quoted 71 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 7 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.

Quoted 10 lines

Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.

After some more thinking I've came to conclusion that the patch as proposed is the proper way forward. It is easier to follow if the unwinding of state changes are local to the function.

With the proposed simplification by Willy

Acked-by Michal Hocko <[email protected]>

Signature · 2 lines

Michal Hocko
SUSE Labs

Waiman Long

author

2021-01-25 18:24 UTC #17 lore raw

Quoted 95 lines · Michal Hocko

Quoted 84 lines · Michal Hocko
Quoted 72 lines · Matthew Wilcox
Quoted 71 lines · Michal Hocko
Quoted 70 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
   [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
   [ 1570.068333] pages's memcg:ffff8889a4116000
   [ 1570.068343] ------------[ cut here ]------------
   [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
   [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
   [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
   [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
   [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
     :
   [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
   [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
   [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
   [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
   [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
   [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
   [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
   [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
   [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
   [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
   [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
   [ 1570.068402] PKRU: 55555554
   [ 1570.068404] Call Trace:
   [ 1570.068407]  mem_cgroup_charge+0x175/0x770
   [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
   [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
   [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
   [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
   [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
   [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
   [ 1570.068693]  read_pages+0x5b1/0xc40
   [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
   [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
   [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
   [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
   [ 1570.068872]  new_sync_read+0x3af/0x610
   [ 1570.068901]  vfs_read+0x339/0x4b0
   [ 1570.068909]  ksys_read+0xf1/0x1c0
   [ 1570.068920]  do_syscall_64+0x33/0x40
   [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
   [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 6 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.
Quoted 10 lines
Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.

Quoted 3 lines

After some more thinking I've came to conclusion that the patch as proposed is the proper way forward. It is easier to follow if the unwinding of state changes are local to the function.

I think so. It is easier to understand if the charge and uncharge functions are grouped together in the same function.

Quoted 3 lines

With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>

Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to

-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);

That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.

Cheers,
Longman

Matthew Wilcox

maintainer

2021-01-25 18:31 UTC #18 lore raw

Quoted 109 lines · Waiman Long

Quoted 95 lines · Michal Hocko
Quoted 84 lines · Michal Hocko
Quoted 72 lines · Matthew Wilcox
Quoted 71 lines · Michal Hocko
Quoted 70 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
   [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
   [ 1570.068333] pages's memcg:ffff8889a4116000
   [ 1570.068343] ------------[ cut here ]------------
   [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
   [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
   [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
   [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
   [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
     :
   [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
   [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
   [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
   [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
   [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
   [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
   [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
   [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
   [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
   [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
   [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
   [ 1570.068402] PKRU: 55555554
   [ 1570.068404] Call Trace:
   [ 1570.068407]  mem_cgroup_charge+0x175/0x770
   [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
   [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
   [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
   [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
   [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
   [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
   [ 1570.068693]  read_pages+0x5b1/0xc40
   [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
   [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
   [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
   [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
   [ 1570.068872]  new_sync_read+0x3af/0x610
   [ 1570.068901]  vfs_read+0x339/0x4b0
   [ 1570.068909]  ksys_read+0xf1/0x1c0
   [ 1570.068920]  do_syscall_64+0x33/0x40
   [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
   [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 6 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.
Quoted 10 lines
Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.
Quoted 3 lines
After some more thinking I've came to conclusion that the patch as proposed is the proper way forward. It is easier to follow if the unwinding of state changes are local to the function.
I think so. It is easier to understand if the charge and uncharge functions are grouped together in the same function.
Quoted 3 lines
With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>
Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to

Quoted 6 lines

-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);
That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.

See earlier emails to Mike suggesting that the accounting of secretmem here is wrong.

Waiman Long

author

2021-01-25 18:45 UTC #19 lore raw

Quoted 108 lines · Matthew Wilcox

Quoted 107 lines · Waiman Long
Quoted 95 lines · Michal Hocko
Quoted 84 lines · Michal Hocko
Quoted 72 lines · Matthew Wilcox
Quoted 71 lines · Michal Hocko
Quoted 70 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
    [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
    [ 1570.068333] pages's memcg:ffff8889a4116000
    [ 1570.068343] ------------[ cut here ]------------
    [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
    [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
    [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
    [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
    [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
      :
    [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
    [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
    [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
    [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
    [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
    [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
    [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
    [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
    [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
    [ 1570.068402] PKRU: 55555554
    [ 1570.068404] Call Trace:
    [ 1570.068407]  mem_cgroup_charge+0x175/0x770
    [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
    [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
    [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
    [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
    [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
    [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
    [ 1570.068693]  read_pages+0x5b1/0xc40
    [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
    [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
    [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
    [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
    [ 1570.068872]  new_sync_read+0x3af/0x610
    [ 1570.068901]  vfs_read+0x339/0x4b0
    [ 1570.068909]  ksys_read+0xf1/0x1c0
    [ 1570.068920]  do_syscall_64+0x33/0x40
    [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
    [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 6 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.
Quoted 10 lines
Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.
Quoted 3 lines
After some more thinking I've came to conclusion that the patch as proposed is the proper way forward. It is easier to follow if the unwinding of state changes are local to the function.
I think so. It is easier to understand if the charge and uncharge functions are grouped together in the same function.
Quoted 2 lines
With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>
Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to

Quoted 9 lines

Quoted 6 lines
-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);
That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.
See earlier emails to Mike suggesting that the accounting of secretmem here is wrong.

I am not in the secretmem discussion and so I am not aware of the issue with that patchset. Anyway, my goal is to minimize potential conflict with other patches.

Cheers,
Longman

Michal Hocko

2021-01-25 18:31 UTC #20 lore raw

In reply to Waiman Long

Quoted 1 line · Waiman Long

On 1/25/21 1:14 PM, Michal Hocko wrote:

[...]

Quoted 6 lines

Quoted 2 lines
With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>
Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to

This is obviously a fix and I believe should go in the next rc while secretmem is a new feature which should wait at least to the next merge window.

Quoted 10 lines

-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);
That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.
Cheers,
Longman

Signature · 2 lines

Michal Hocko
SUSE Labs

Johannes Weiner

2021-01-25 18:52 UTC #21 lore raw

In reply to Waiman Long

Quoted 109 lines · Waiman Long

Quoted 95 lines · Michal Hocko
Quoted 84 lines · Michal Hocko
Quoted 72 lines · Matthew Wilcox
Quoted 71 lines · Michal Hocko
Quoted 70 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
   [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
   [ 1570.068333] pages's memcg:ffff8889a4116000
   [ 1570.068343] ------------[ cut here ]------------
   [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
   [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
   [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
   [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
   [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
     :
   [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
   [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
   [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
   [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
   [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
   [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
   [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
   [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
   [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
   [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
   [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
   [ 1570.068402] PKRU: 55555554
   [ 1570.068404] Call Trace:
   [ 1570.068407]  mem_cgroup_charge+0x175/0x770
   [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
   [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
   [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
   [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
   [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
   [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
   [ 1570.068693]  read_pages+0x5b1/0xc40
   [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
   [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
   [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
   [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
   [ 1570.068872]  new_sync_read+0x3af/0x610
   [ 1570.068901]  vfs_read+0x339/0x4b0
   [ 1570.068909]  ksys_read+0xf1/0x1c0
   [ 1570.068920]  do_syscall_64+0x33/0x40
   [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
   [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 6 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.
Quoted 10 lines
Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.
Quoted 3 lines
After some more thinking I've came to conclusion that the patch as proposed is the proper way forward. It is easier to follow if the unwinding of state changes are local to the function.
I think so. It is easier to understand if the charge and uncharge functions are grouped together in the same function.
Quoted 3 lines
With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>
Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to

Quoted 6 lines

-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);
That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.

The variable works for me.

On the other hand, as Michal points out, the uncharge function will be called again on the page when it's being freed (in non-fscache cases), so you're already relying on being able to call it on any page - charged, uncharged, never charged. It would be fine to call it unconditionally in the error path. Aesthetic preference, I guess.

Waiman Long

author

2021-01-25 18:57 UTC #22 lore raw

Quoted 114 lines · Johannes Weiner

Quoted 107 lines · Waiman Long
Quoted 95 lines · Michal Hocko
Quoted 84 lines · Michal Hocko
Quoted 72 lines · Matthew Wilcox
Quoted 71 lines · Michal Hocko
Quoted 70 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
    [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
    [ 1570.068333] pages's memcg:ffff8889a4116000
    [ 1570.068343] ------------[ cut here ]------------
    [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
    [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
    [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
    [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
    [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
      :
    [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
    [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
    [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
    [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
    [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
    [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
    [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
    [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
    [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
    [ 1570.068402] PKRU: 55555554
    [ 1570.068404] Call Trace:
    [ 1570.068407]  mem_cgroup_charge+0x175/0x770
    [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
    [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
    [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
    [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
    [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
    [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
    [ 1570.068693]  read_pages+0x5b1/0xc40
    [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
    [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
    [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
    [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
    [ 1570.068872]  new_sync_read+0x3af/0x610
    [ 1570.068901]  vfs_read+0x339/0x4b0
    [ 1570.068909]  ksys_read+0xf1/0x1c0
    [ 1570.068920]  do_syscall_64+0x33/0x40
    [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
    [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 6 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.
Quoted 10 lines
Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.
Quoted 3 lines
After some more thinking I've came to conclusion that the patch as proposed is the proper way forward. It is easier to follow if the unwinding of state changes are local to the function.
I think so. It is easier to understand if the charge and uncharge functions are grouped together in the same function.
Quoted 2 lines
With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>
Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to
Quoted 6 lines
-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);
That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.

Quoted 7 lines

The variable works for me.
On the other hand, as Michal points out, the uncharge function will be called again on the page when it's being freed (in non-fscache cases), so you're already relying on being able to call it on any page - charged, uncharged, never charged. It would be fine to call it unconditionally in the error path. Aesthetic preference, I guess.

That may be true. However, I haven't fully studied how the huge page memory accounting work to make sure the uncharge function can be called for huge pages. So I will keep the current code for now.

Thanks,
Longman

Michal Hocko

2021-01-26 08:01 UTC #23 lore raw

Quoted 3 lines · Waiman Long

Quoted 2 lines · Johannes Weiner
Quoted 1 line · Waiman Long
On 1/25/21 1:14 PM, Michal Hocko wrote:

[...]

Quoted 12 lines

Quoted 12 lines
Quoted 12 lines
Quoted 2 lines
With the proposed simplification by Willy
Acked-by Michal Hocko <[email protected]>
Thank for the ack. However, I am a bit confused about what you mean by simplification. There is another linux-next patch that changes the condition for mem_cgroup_charge() to
-       if (!huge) {
+       if (!huge && !page_is_secretmem(page)) {
                error = mem_cgroup_charge(page, current->mm, gfp);
That is the main reason why I introduced the boolean variable as I don't want to call the external page_is_secretmem() function twice.

Quoted 7 lines

Quoted 7 lines
The variable works for me.
On the other hand, as Michal points out, the uncharge function will be called again on the page when it's being freed (in non-fscache cases), so you're already relying on being able to call it on any page - charged, uncharged, never charged. It would be fine to call it unconditionally in the error path. Aesthetic preference, I guess.

Yes aesthetic preference... Just compare to

mm/filemap.c +4 -1

index 5c9d564317a5..7aa05420107e 100644
@@ -896,11 +896,14 @@ noinline int __add_to_page_cache_locked(struct page *page,
 
 	if (xas_error(&xas)) {
 		error = xas_error(&xas);
-		goto error;
+		goto error_uncharge;
 	}
 
 	trace_mm_filemap_add_to_page_cache(page);
 	return 0;
+error_uncharge:
+	/* memcg will ignore uncharged pages */
+	mem_cgroup_uncharge(page);
 error:
 	page->mapping = NULL;
 	/* Leave page->index set: truncation relies upon it */

which resembles our usual state unwinding style much more.

Quoted 3 lines

That may be true. However, I haven't fully studied how the huge page memory accounting work to make sure the uncharge function can be called for huge pages.

... but this is rather lame argument to make, don't you think. This sounds like a ducktaping engineering to me. Over time this leads to a terrible code. Seriously!

All that being said I do not want to block this or bother people with more emails but geez

Signature · 2 lines

Michal Hocko
SUSE Labs

Johannes Weiner

2021-01-25 18:43 UTC #24 lore raw

In reply to Michal Hocko

Quoted 89 lines · Michal Hocko

Quoted 74 lines · Matthew Wilcox
Quoted 73 lines · Michal Hocko
Quoted 71 lines · Waiman Long
Quoted 61 lines · Michal Hocko
Quoted 52 lines · Waiman Long
The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:
  [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
  [ 1570.068333] pages's memcg:ffff8889a4116000
  [ 1570.068343] ------------[ cut here ]------------
  [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
  [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
  [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
  [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
  [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
    :
  [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
  [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
  [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
  [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
  [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
  [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
  [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
  [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
  [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [ 1570.068402] PKRU: 55555554
  [ 1570.068404] Call Trace:
  [ 1570.068407]  mem_cgroup_charge+0x175/0x770
  [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
  [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
  [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
  [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
  [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
  [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
  [ 1570.068693]  read_pages+0x5b1/0xc40
  [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
  [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
  [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
  [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
  [ 1570.068872]  new_sync_read+0x3af/0x610
  [ 1570.068901]  vfs_read+0x339/0x4b0
  [ 1570.068909]  ksys_read+0xf1/0x1c0
  [ 1570.068920]  do_syscall_64+0x33/0x40
  [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  [ 1570.068930] RIP: 0033:0x7ff039135595
Before that commit, there was a try_charge() and commit_charge() in __add_to_page_cache_locked(). These 2 separated charge functions were replaced by a single mem_cgroup_charge(). However, it forgot to add a matching mem_cgroup_uncharge() when the xarray insertion failed with the page released back to the pool. Fix this by adding a mem_cgroup_uncharge() call when insertion error happens.
Quoted 2 lines
Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>
OK, this is indeed a subtle bug. The patch aimed at simplifying the charge lifetime so that users do not really have to think about when to uncharge as that happens when the page is freed. fscache somehow breaks that assumption because it doesn't free up pages but it keeps some of them in the cache.
Quoted 5 lines
I have tried to wrap my head around the cached object life time in fscache but failed and got lost in the maze. Is this the only instance of the problem? Would it make more sense to explicitly handle charges in the fscache code or there are other potential users to fall into this trap?
There may be other places that have similar problem. I focus on the filemap.c case as I have a test case that can reliably produce the bug splat. This patch does fix it for my test case.
Quoted 7 lines
Quoted 4 lines
I believe this needs a more general fix than catching a random places which you can trigger. Would it make more sense to address this at the fscache level and always make sure that a page returned to the pool is always uncharged instead?
I believe you mean "page cache" -- there is a separate thing called 'fscache' which is used to cache network filesystems.
Yes, I really had fscache in mind because it does have an "unusual" page life time rules.
Quoted 2 lines
I don't understand the memcg code at all, so I have no useful feedback on what you're saying other than this.

Quoted 10 lines

Well the memcg accounting rules after the rework should have simplified the API usage for most users. You will get memory charged when it is used and it will go away when the page is freed. If a page is not really freed in some cases and it can be reused then it doesn't really fit into this scheme automagically. I do undestand that this puts some additional burden on those special cases. I am not really sure what is the right way here myself but considering there might be other similar cases like that I would lean towards special casing where the pool is implemented. I would expect there is some state to be maintain for that purpose already.

FWIW, khugepaged does a similar type of page recycling, where when one virtual area fails to collapse, the scanner caches the physical page and tries to reuse it for the next area. See the mem_cgroup_uncharge() call in collapse_huge_page().

While it's nice to be able to leave the uncharge to the free call, in reuse cases like this I don't think it's a problem to have a matching uncharge.

Johannes Weiner

Acked-by on v1 1/1

2021-01-26 10:18 UTC #25 lore raw

In reply to Waiman Long

Quoted 52 lines · Waiman Long

The commit 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API") introduced a bug in __add_to_page_cache_locked() causing the following splat:

 [ 1570.068330] page dumped because: VM_BUG_ON_PAGE(page_memcg(page))
 [ 1570.068333] pages's memcg:ffff8889a4116000
 [ 1570.068343] ------------[ cut here ]------------
 [ 1570.068346] kernel BUG at mm/memcontrol.c:2924!
 [ 1570.068355] invalid opcode: 0000 [#1] SMP KASAN PTI
 [ 1570.068359] CPU: 35 PID: 12345 Comm: cat Tainted: G S      W I       5.11.0-rc4-debug+ #1
 [ 1570.068363] Hardware name: HP HP Z8 G4 Workstation/81C7, BIOS P60 v01.25 12/06/2017
 [ 1570.068365] RIP: 0010:commit_charge+0xf4/0x130
   :
 [ 1570.068375] RSP: 0018:ffff8881b38d70e8 EFLAGS: 00010286
 [ 1570.068379] RAX: 0000000000000000 RBX: ffffea00260ddd00 RCX: 0000000000000027
 [ 1570.068382] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88907ebe05a8
 [ 1570.068384] RBP: ffffea00260ddd00 R08: ffffed120fd7c0b6 R09: ffffed120fd7c0b6
 [ 1570.068386] R10: ffff88907ebe05ab R11: ffffed120fd7c0b5 R12: ffffea00260ddd38
 [ 1570.068389] R13: ffff8889a4116000 R14: ffff8889a4116000 R15: 0000000000000001
 [ 1570.068391] FS:  00007ff039638680(0000) GS:ffff88907ea00000(0000) knlGS:0000000000000000
 [ 1570.068394] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [ 1570.068396] CR2: 00007f36f354cc20 CR3: 00000008a0126006 CR4: 00000000007706e0
 [ 1570.068398] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 [ 1570.068400] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 [ 1570.068402] PKRU: 55555554
 [ 1570.068404] Call Trace:
 [ 1570.068407]  mem_cgroup_charge+0x175/0x770
 [ 1570.068413]  __add_to_page_cache_locked+0x712/0xad0
 [ 1570.068439]  add_to_page_cache_lru+0xc5/0x1f0
 [ 1570.068461]  cachefiles_read_or_alloc_pages+0x895/0x2e10 [cachefiles]
 [ 1570.068524]  __fscache_read_or_alloc_pages+0x6c0/0xa00 [fscache]
 [ 1570.068540]  __nfs_readpages_from_fscache+0x16d/0x630 [nfs]
 [ 1570.068585]  nfs_readpages+0x24e/0x540 [nfs]
 [ 1570.068693]  read_pages+0x5b1/0xc40
 [ 1570.068711]  page_cache_ra_unbounded+0x460/0x750
 [ 1570.068729]  generic_file_buffered_read_get_pages+0x290/0x1710
 [ 1570.068756]  generic_file_buffered_read+0x2a9/0xc30
 [ 1570.068832]  nfs_file_read+0x13f/0x230 [nfs]
 [ 1570.068872]  new_sync_read+0x3af/0x610
 [ 1570.068901]  vfs_read+0x339/0x4b0
 [ 1570.068909]  ksys_read+0xf1/0x1c0
 [ 1570.068920]  do_syscall_64+0x33/0x40
 [ 1570.068926]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
 [ 1570.068930] RIP: 0033:0x7ff039135595

Quoted 2 lines

Fixes 3fea5a499d57 ("mm: memcontrol: convert page cache to a new mem_cgroup_charge() API")
Signed-off-by Waiman Long <[email protected]>

Thanks Waiman.

Acked-by Johannes Weiner <[email protected]>

Since this crashes the kernel, we should also add

Cc [email protected] # 5.8+