| Message ID | 20230129024451.121590-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memcg: fix NULL pointer in mem_cgroup_track_foreign_dirty() | expand |
On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), Merged in 2017. > hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg > could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could > occurs a NULL pointer dereference, let's do not record the foreign > writebacks for folio memcg is null in mem_cgroup_track_foreign() to > fix it. > > Reported-by: Ma Wupeng <mawupeng1@huawei.com> > Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") Merged in 2019. > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -1688,10 +1688,13 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio, > static inline void mem_cgroup_track_foreign_dirty(struct folio *folio, > struct bdi_writeback *wb) > { > + struct mem_cgroup *memcg; > + > if (mem_cgroup_disabled()) > return; > > - if (unlikely(&folio_memcg(folio)->css != wb->memcg_css)) > + memcg = folio_memcg(folio); > + if (unlikely(memcg && &memcg->css != wb->memcg_css)) > mem_cgroup_track_foreign_dirty_slowpath(folio, wb); > } Has this null deref actually been observed, or is this from code inspection? (This is why it's nice to include the Link: after a Reported-by!) Do we have any theories why this took so many years to surface? I'm confused about the mention of 18365225f044, but the Fixes: target is a different commit. Please explain this? Do you think the fix should be backported into earlier -stable kernels? If so, it will need some rework due to the subsequent folio conversion.
On 2023/1/30 5:48, Andrew Morton wrote: > On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), > > Merged in 2017. > >> hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg >> could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could >> occurs a NULL pointer dereference, let's do not record the foreign >> writebacks for folio memcg is null in mem_cgroup_track_foreign() to >> fix it. >> >> Reported-by: Ma Wupeng <mawupeng1@huawei.com> >> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") > > Merged in 2019. > >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -1688,10 +1688,13 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio, >> static inline void mem_cgroup_track_foreign_dirty(struct folio *folio, >> struct bdi_writeback *wb) >> { >> + struct mem_cgroup *memcg; >> + >> if (mem_cgroup_disabled()) >> return; >> >> - if (unlikely(&folio_memcg(folio)->css != wb->memcg_css)) >> + memcg = folio_memcg(folio); >> + if (unlikely(memcg && &memcg->css != wb->memcg_css)) >> mem_cgroup_track_foreign_dirty_slowpath(folio, wb); >> } > > Has this null deref actually been observed, or is this from code > inspection? (This is why it's nice to include the Link: after a > Reported-by!) It does occurs in our internal test and report by wupeng(based v5.10), BUG: KASAN: user-memory-access in mem_cgroup_track_foreign_dirty_slowpath+0xc0/0x480 mm/memcontrol.c:4708 Read of size 8 at addr 0000000000001000 by task syz-executor.2/28325 CPU: 2 PID: 28325 Comm: syz-executor.2 Tainted: G W 5.10.0-03333-g48e46a146cbc-dirty #1 Hardware name: linux,dummy-virt (DT) Call trace: ... mem_cgroup_track_foreign_dirty_slowpath+0xc0/0x480 mm/memcontrol.c:4708 mem_cgroup_track_foreign_dirty include/linux/memcontrol.h:1880 [inline] account_page_dirtied+0x9a0/0xa90 mm/page-writeback.c:2436 __set_page_dirty+0x1f8/0x4b0 fs/buffer.c:608 __set_page_dirty_buffers+0x3d0/0x550 fs/buffer.c:668 set_page_dirty+0x158/0x500 mm/page-writeback.c:2575 filemap_page_mkwrite+0x3dc/0x490 mm/filemap.c:3224 do_page_mkwrite+0xc4/0x3d0 mm/memory.c:2786 wp_page_shared+0x14c/0x980 mm/memory.c:3118 do_wp_page+0x930/0xbc0 mm/memory.c:3219 handle_pte_fault+0x5e0/0x630 mm/memory.c:4570 __handle_mm_fault+0x41c/0x910 mm/memory.c:4690 handle_mm_fault+0x25c/0x484 mm/memory.c:4788 __do_page_fault arch/arm64/mm/fault.c:440 [inline] do_page_fault+0x3ac/0x9d4 arch/arm64/mm/fault.c:539 > > Do we have any theories why this took so many years to surface? After google, I found a similar issue[1], maybe hwpoison/mem_cgroup_track_foreign_dirty concurrency is uncommon. [1] https://syzkaller.appspot.com/bug?extid=0c84bf23aed8ee0d8399 > > I'm confused about the mention of 18365225f044, but the Fixes: target > is a different commit. Please explain this? 18365225f044 said that it will uncharge it manually from its memcg in hwpison handler, but when the new feature "writeback, memcg: Implement foreign dirty flushing" is introduced, we don't consider this, when folio's memcg is cleared by hwpison handler, the issue occurs. > > Do you think the fix should be backported into earlier -stable kernels? it's better to send stable kernel. > If so, it will need some rework due to the subsequent folio > conversion. When the patch is merged, I could refresh and send to stable maillist. > > >
On Mon 30-01-23 09:16:13, Kefeng Wang wrote: > > > On 2023/1/30 5:48, Andrew Morton wrote: > > On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > > As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), > > > > Merged in 2017. > > > > > hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg > > > could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could > > > occurs a NULL pointer dereference, let's do not record the foreign > > > writebacks for folio memcg is null in mem_cgroup_track_foreign() to > > > fix it. > > > > > > Reported-by: Ma Wupeng <mawupeng1@huawei.com> > > > Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") > > > > Merged in 2019. > > > > > --- a/include/linux/memcontrol.h > > > +++ b/include/linux/memcontrol.h > > > @@ -1688,10 +1688,13 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio, > > > static inline void mem_cgroup_track_foreign_dirty(struct folio *folio, > > > struct bdi_writeback *wb) > > > { > > > + struct mem_cgroup *memcg; > > > + > > > if (mem_cgroup_disabled()) > > > return; > > > - if (unlikely(&folio_memcg(folio)->css != wb->memcg_css)) > > > + memcg = folio_memcg(folio); > > > + if (unlikely(memcg && &memcg->css != wb->memcg_css)) > > > mem_cgroup_track_foreign_dirty_slowpath(folio, wb); > > > } > > > > Has this null deref actually been observed, or is this from code > > inspection? (This is why it's nice to include the Link: after a > > Reported-by!) > > It does occurs in our internal test and report by wupeng(based v5.10), > > BUG: KASAN: user-memory-access in > mem_cgroup_track_foreign_dirty_slowpath+0xc0/0x480 mm/memcontrol.c:4708 > Read of size 8 at addr 0000000000001000 by task syz-executor.2/28325 > > CPU: 2 PID: 28325 Comm: syz-executor.2 Tainted: G W > 5.10.0-03333-g48e46a146cbc-dirty #1 > Hardware name: linux,dummy-virt (DT) > Call trace: > ... > mem_cgroup_track_foreign_dirty_slowpath+0xc0/0x480 mm/memcontrol.c:4708 > mem_cgroup_track_foreign_dirty include/linux/memcontrol.h:1880 [inline] > account_page_dirtied+0x9a0/0xa90 mm/page-writeback.c:2436 > __set_page_dirty+0x1f8/0x4b0 fs/buffer.c:608 > __set_page_dirty_buffers+0x3d0/0x550 fs/buffer.c:668 > set_page_dirty+0x158/0x500 mm/page-writeback.c:2575 > filemap_page_mkwrite+0x3dc/0x490 mm/filemap.c:3224 > do_page_mkwrite+0xc4/0x3d0 mm/memory.c:2786 > wp_page_shared+0x14c/0x980 mm/memory.c:3118 > do_wp_page+0x930/0xbc0 mm/memory.c:3219 > handle_pte_fault+0x5e0/0x630 mm/memory.c:4570 > __handle_mm_fault+0x41c/0x910 mm/memory.c:4690 > handle_mm_fault+0x25c/0x484 mm/memory.c:4788 > __do_page_fault arch/arm64/mm/fault.c:440 [inline] > do_page_fault+0x3ac/0x9d4 arch/arm64/mm/fault.c:539 Just to make sure I understand. The page has been hwpoisoned, uncharged but stayed in the page cache so a next page fault on the address has blowned up? Say we address the NULL memcg case. What is the resulting behavior? Doesn't userspace access a poisoned page and get a silend memory corruption?
On 2023/1/30 16:48, Michal Hocko wrote: > On Mon 30-01-23 09:16:13, Kefeng Wang wrote: >> >> >> On 2023/1/30 5:48, Andrew Morton wrote: >>> On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >>> >>>> As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), >>> >>> Merged in 2017. >>> >>>> hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg >>>> could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could >>>> occurs a NULL pointer dereference, let's do not record the foreign >>>> writebacks for folio memcg is null in mem_cgroup_track_foreign() to >>>> fix it. >>>> >>>> Reported-by: Ma Wupeng <mawupeng1@huawei.com> >>>> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") >>> >>> Merged in 2019. >>> ... > > Just to make sure I understand. The page has been hwpoisoned, uncharged > but stayed in the page cache so a next page fault on the address has blowned > up? > > Say we address the NULL memcg case. What is the resulting behavior? > Doesn't userspace access a poisoned page and get a silend memory > corruption? + Yang Shi Check previous link[1], seems that it is a known issue, and there is a TODO list for storage backed filesystems from Yang. [1] https://lore.kernel.org/all/20211020210755.23964-6-shy828301@gmail.com/T/#m1d40559ca2dcf94396df5369214288f69dec379b
On Mon 30-01-23 20:20:16, Kefeng Wang wrote: > > > On 2023/1/30 16:48, Michal Hocko wrote: > > On Mon 30-01-23 09:16:13, Kefeng Wang wrote: > > > > > > > > > On 2023/1/30 5:48, Andrew Morton wrote: > > > > On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > > > > > > As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), > > > > > > > > Merged in 2017. > > > > > > > > > hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg > > > > > could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could > > > > > occurs a NULL pointer dereference, let's do not record the foreign > > > > > writebacks for folio memcg is null in mem_cgroup_track_foreign() to > > > > > fix it. > > > > > > > > > > Reported-by: Ma Wupeng <mawupeng1@huawei.com> > > > > > Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") > > > > > > > > Merged in 2019. > > > > > ... > > > > Just to make sure I understand. The page has been hwpoisoned, uncharged > > but stayed in the page cache so a next page fault on the address has blowned > > up? > > > > Say we address the NULL memcg case. What is the resulting behavior? > > Doesn't userspace access a poisoned page and get a silend memory > > corruption? > > + Yang Shi > > Check previous link[1], seems that it is a known issue, and there is a TODO > list for storage backed filesystems from Yang. OK, so IIUC this patch will just help the test to not blow up but it will not allow the test to behave consistently. From my past experience the hwpoisoning is not really something that any production environment should be relying on working properly. But this patch is straightforward so no objection from me. Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > [1] https://lore.kernel.org/all/20211020210755.23964-6-shy828301@gmail.com/T/#m1d40559ca2dcf94396df5369214288f69dec379b
On Mon, Jan 30, 2023 at 4:20 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > On 2023/1/30 16:48, Michal Hocko wrote: > > On Mon 30-01-23 09:16:13, Kefeng Wang wrote: > >> > >> > >> On 2023/1/30 5:48, Andrew Morton wrote: > >>> On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >>> > >>>> As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), > >>> > >>> Merged in 2017. > >>> > >>>> hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg > >>>> could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could > >>>> occurs a NULL pointer dereference, let's do not record the foreign > >>>> writebacks for folio memcg is null in mem_cgroup_track_foreign() to > >>>> fix it. > >>>> > >>>> Reported-by: Ma Wupeng <mawupeng1@huawei.com> > >>>> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") > >>> > >>> Merged in 2019. > >>> > ... > > > > Just to make sure I understand. The page has been hwpoisoned, uncharged > > but stayed in the page cache so a next page fault on the address has blowned > > up? > > > > Say we address the NULL memcg case. What is the resulting behavior? > > Doesn't userspace access a poisoned page and get a silend memory > > corruption? > > + Yang Shi > > Check previous link[1], seems that it is a known issue, and there is a > TODO list for storage backed filesystems from Yang. For tmpfs and hugetlbfs, the page cache still stay in page cache, the later page fault will handle the case gracefully. Other real storage backed filesystem will have page cache truncated. The page cache will be uncharged before truncate. If the truncate fails, we may end up in this case. > > > [1] > https://lore.kernel.org/all/20211020210755.23964-6-shy828301@gmail.com/T/#m1d40559ca2dcf94396df5369214288f69dec379b
On Mon 30-01-23 11:30:47, Yang Shi wrote: > On Mon, Jan 30, 2023 at 4:20 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > > > > > On 2023/1/30 16:48, Michal Hocko wrote: > > > On Mon 30-01-23 09:16:13, Kefeng Wang wrote: > > >> > > >> > > >> On 2023/1/30 5:48, Andrew Morton wrote: > > >>> On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > >>> > > >>>> As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), > > >>> > > >>> Merged in 2017. > > >>> > > >>>> hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg > > >>>> could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could > > >>>> occurs a NULL pointer dereference, let's do not record the foreign > > >>>> writebacks for folio memcg is null in mem_cgroup_track_foreign() to > > >>>> fix it. > > >>>> > > >>>> Reported-by: Ma Wupeng <mawupeng1@huawei.com> > > >>>> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") > > >>> > > >>> Merged in 2019. > > >>> > > ... > > > > > > Just to make sure I understand. The page has been hwpoisoned, uncharged > > > but stayed in the page cache so a next page fault on the address has blowned > > > up? > > > > > > Say we address the NULL memcg case. What is the resulting behavior? > > > Doesn't userspace access a poisoned page and get a silend memory > > > corruption? > > > > + Yang Shi > > > > Check previous link[1], seems that it is a known issue, and there is a > > TODO list for storage backed filesystems from Yang. > > For tmpfs and hugetlbfs, the page cache still stay in page cache, the > later page fault will handle the case gracefully. Other real storage > backed filesystem will have page cache truncated. > > The page cache will be uncharged before truncate. If the truncate > fails, we may end up in this case. This would be a good addendum to the changelog. What would be a typical failure in the truncation path? > > > > > > [1] > > https://lore.kernel.org/all/20211020210755.23964-6-shy828301@gmail.com/T/#m1d40559ca2dcf94396df5369214288f69dec379b
On Wed, Feb 1, 2023 at 12:07 AM Michal Hocko <mhocko@suse.com> wrote: > > On Mon 30-01-23 11:30:47, Yang Shi wrote: > > On Mon, Jan 30, 2023 at 4:20 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > > > > > > > > > On 2023/1/30 16:48, Michal Hocko wrote: > > > > On Mon 30-01-23 09:16:13, Kefeng Wang wrote: > > > >> > > > >> > > > >> On 2023/1/30 5:48, Andrew Morton wrote: > > > >>> On Sun, 29 Jan 2023 10:44:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > >>> > > > >>>> As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), > > > >>> > > > >>> Merged in 2017. > > > >>> > > > >>>> hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg > > > >>>> could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could > > > >>>> occurs a NULL pointer dereference, let's do not record the foreign > > > >>>> writebacks for folio memcg is null in mem_cgroup_track_foreign() to > > > >>>> fix it. > > > >>>> > > > >>>> Reported-by: Ma Wupeng <mawupeng1@huawei.com> > > > >>>> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") > > > >>> > > > >>> Merged in 2019. > > > >>> > > > ... > > > > > > > > Just to make sure I understand. The page has been hwpoisoned, uncharged > > > > but stayed in the page cache so a next page fault on the address has blowned > > > > up? > > > > > > > > Say we address the NULL memcg case. What is the resulting behavior? > > > > Doesn't userspace access a poisoned page and get a silend memory > > > > corruption? > > > > > > + Yang Shi > > > > > > Check previous link[1], seems that it is a known issue, and there is a > > > TODO list for storage backed filesystems from Yang. > > > > For tmpfs and hugetlbfs, the page cache still stay in page cache, the > > later page fault will handle the case gracefully. Other real storage > > backed filesystem will have page cache truncated. > > > > The page cache will be uncharged before truncate. If the truncate > > fails, we may end up in this case. > > This would be a good addendum to the changelog. What would be a typical > failure in the truncation path? For memory failure path, there may be a couple of cases, for example, page is not for a regular file (maybe directory), fail to release buffers, etc. > > > > > > > > > > [1] > > > https://lore.kernel.org/all/20211020210755.23964-6-shy828301@gmail.com/T/#m1d40559ca2dcf94396df5369214288f69dec379b > > -- > Michal Hocko > SUSE Labs
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index eb6e5b18e1ad..35478695cabf 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -1688,10 +1688,13 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio, static inline void mem_cgroup_track_foreign_dirty(struct folio *folio, struct bdi_writeback *wb) { + struct mem_cgroup *memcg; + if (mem_cgroup_disabled()) return; - if (unlikely(&folio_memcg(folio)->css != wb->memcg_css)) + memcg = folio_memcg(folio); + if (unlikely(memcg && &memcg->css != wb->memcg_css)) mem_cgroup_track_foreign_dirty_slowpath(folio, wb); }
As commit 18365225f044 ("hwpoison, memcg: forcibly uncharge LRU pages"), hwpoison will forcibly uncharg a LRU hwpoisoned page, the folio_memcg could be NULl, then, mem_cgroup_track_foreign_dirty_slowpath() could occurs a NULL pointer dereference, let's do not record the foreign writebacks for folio memcg is null in mem_cgroup_track_foreign() to fix it. Reported-by: Ma Wupeng <mawupeng1@huawei.com> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- include/linux/memcontrol.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)