[PATCH] nilfs2: prevent double insertion of b_assoc_buffers in dirty buffer lookup

[wuyankun]

syzbot reported list corruption caused by double list_add_tail() on
bh->b_assoc_buffers in nilfs_lookup_dirty_data_buffers().

A buffer_head can still be dirty and not under async write while already
linked on another association list. Add list state checks before enqueueing
in both data and node dirty buffer scanners to avoid re-adding already
linked nodes.

Reported-by: syzbot+c37bed...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=c37bed40868932d790e9
Signed-off-by: wuyankun <wuya...@uniontech.com>
---
fs/nilfs2/segment.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index 1491a4d4b1e1..09202d155903 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -741,6 +741,8 @@ static size_t nilfs_lookup_dirty_data_buffers(struct inode *inode,
do {
if (!buffer_dirty(bh) || buffer_async_write(bh))
continue;
+ if (!list_empty(&bh->b_assoc_buffers))
+ continue;
get_bh(bh);
list_add_tail(&bh->b_assoc_buffers, listp);
ndirties++;
@@ -779,7 +781,8 @@ static void nilfs_lookup_dirty_node_buffers(struct inode *inode,
bh = head = folio_buffers(fbatch.folios[i]);
do {
if (buffer_dirty(bh) &&
- !buffer_async_write(bh)) {
+ !buffer_async_write(bh) &&
+ list_empty(&bh->b_assoc_buffers)) {
get_bh(bh);
list_add_tail(&bh->b_assoc_buffers,
listp);
--
2.20.1

[wuyankun]

[Ryusuke Konishi]

Thank you for the patch.

This patch adds a fix to forcibly avoid double registration of a
buffer head to the list at the list manipulation level.

However, we need to clarify why that happened in the first place, and
what caused the state inconsistency that led to this double
registration.

The search routine for dirty buffers performs a gang lookup for dirty
folios, registering them to the list in ascending order of their
offsets in the page cache.

Furthermore, the construction and destruction of this list take place
inside the log writer, which operates exclusively. Whether the write
succeeds or fails, the list is released by nilfs_release_buffers()
(called via nilfs_destroy_logs()). Therefore, double registration
normally does not occur, meaning there must be an oversight somewhere.

As this issue arises from calls made via the GC ioctl
(nilfs_ioctl_clean_segments()), the underlying implementation flaw
likely lies there.

Since it seems reproducible with the syzbot reproducer, I will also
trace what is happening to verify if this fix direction is
appropriate.

Thanks,
Ryusuke Konishi