Message-ID: <qfizhbe5rwzddwnoekr6xjy3gozbqbtl64c5xmfeuudxvficmv@onazesxv4ur6>
Date: Fri, 31 Oct 2025 10:30:01 +0800
From: Heming Zhao <heming.zhao@...e.com>
To: Ahmet Eray Karadag <eraykrdg1@...il.com>
Cc: mark@...heh.com, jlbec@...lplan.org, joseph.qi@...ux.alibaba.com, 
	ocfs2-devel@...ts.linux.dev, linux-kernel@...r.kernel.org, david.hunter.linux@...il.com, 
	skhan@...uxfoundation.org, syzbot+b93b65ee321c97861072@...kaller.appspotmail.com, 
	Albin Babu Varghese <albinbabuvarghese20@...il.com>
Subject: Re: [RFC RFT PATCH] ocfs2: Mark inode bad upon validation failure
 during read

On Fri, Oct 31, 2025 at 01:07:56AM +0300, Ahmet Eray Karadag wrote:
> Hi Heming,
> 
> Thanks a lot for the review and comments.
> 
> Regarding the placement of make_bad_inode(), our patch places it in
> ocfs2_read_inode_block_full() because
> ocfs2_validate_inode_block() itself doesn't have access
> to the inode object. I believe this (in the caller) is the
> correct layer, as it seems to match other patterns in OCFS2
> where the caller handles the make_bad_inode upon validation
> failure.

Thanks for pointing that out. I agree with the above comments.
> 
> I had one question about your proposal to combine this patch with
> Albin's [1]. When you mentioned, "We should forbid any write
> operations," were you referring to Albin's read-only check in
> ocfs2_setattr as the mechanism to "forbid" the operation? Or
> were you suggesting we should use the inode size sanity check
> itself (e.g., by converting the BUG_ON to an -EIO return)
> as that mechanism?
> 
> Thanks,
> Eray

The 'forbid' refers to the read-only check in ocfs2_setattr.
We can refer to ext4_setattr(), which calls ext4_emergency_state()
to forbid write operations.

- Heming
> 
> Heming Zhao <heming.zhao@...e.com>, 30 Eki 2025 Per, 10:59 tarihinde şunu yazdı:
> >
> > Hi,
> >
> > In my view, combining this patch and another patch [1] is a complete
> > solution for this bug.
> >
> > According to the oops stack, the FS is already in read-only mode.
> > We should forbid any write operations and then perform the inode
> > sanity check.
> >
> > And I think ocfs2_validate_inode_block is a good place for make_bad_inode().
> >
> > [1]:
> > https://syzkaller.appspot.com/text?tag=Patch&x=1287f614580000
> > - by albinbabuvarghese20@...il.com from:
> >   https://syzkaller.appspot.com/bug?extid=b93b65ee321c97861072
> >
> > Thanks,
> > Heming
> >
> > On Thu, Oct 30, 2025 at 01:57:49AM +0300, Ahmet Eray Karadag wrote:
> > > Potentially triggered by sequences like buffered writes followed by
> > > open(O_DIRECT), can result in an invalid on-disk inode block
> > > (e.g., bad signature). OCFS2 detects this corruption when reading the
> > > inode block via ocfs2_validate_inode_block(), logs "Invalid dinode",
> > > and often switches the filesystem to read-only mode.
> > >
> > > Currently, the function reading the inode block (ocfs2_read_inode_block_full())
> > > fails to call make_bad_inode() upon detecting the validation error.
> > > Because the in-memory inode is not marked bad, subsequent operations
> > > (like ftruncate) proceed erroneously. They eventually reach code
> > > (e.g., ocfs2_truncate_file()) that compares the inconsistent
> > > in-memory size (38639) against the invalid/stale on-disk size (0), leading
> > > to kernel crashes via BUG_ON.
> > >
> > > Fix this by calling make_bad_inode(inode) within the error handling path of
> > > ocfs2_read_inode_block_full() immediately after a block read or validation
> > > error occurs. This ensures VFS is properly notified about the
> > > corrupt inode at the point of detection. Marking the inode bad  allows VFS
> > > to correctly fail subsequent operations targeting this inode early,
> > > preventing kernel panics caused by operating on known inconsistent inode states.
> > >
> > > [RFC]: While this patch prevents the kernel crash triggered by the reproducer,
> > > feedback is requested on whether ocfs2_read_inode_block_full() is the most
> > > appropriate layer to call make_bad_inode(). Should this check perhaps reside
> > > within the caller or should the error propagation be handled differently?:
> > > Input on the best practice for handling this specific VFS inconsistency
> > > within OCFS2 would be appreciated.
> > >
> > > Reported-by: syzbot+b93b65ee321c97861072@...kaller.appspotmail.com
> > > Link: https://syzkaller.appspot.com/bug?extid=b93b65ee321c97861072
> > > Co-developed-by: Albin Babu Varghese <albinbabuvarghese20@...il.com>
> > > Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@...il.com>
> > > Signed-off-by: Ahmet Eray Karadag <eraykrdg1@...il.com>
> > > ---
> > >  fs/ocfs2/inode.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> > > index fcc89856ab95..415ad29ec758 100644
> > > --- a/fs/ocfs2/inode.c
> > > +++ b/fs/ocfs2/inode.c
> > > @@ -1690,6 +1690,8 @@ int ocfs2_read_inode_block_full(struct inode *inode, struct buffer_head **bh,
> > >       rc = ocfs2_read_blocks(INODE_CACHE(inode), OCFS2_I(inode)->ip_blkno,
> > >                              1, &tmp, flags, ocfs2_validate_inode_block);
> > >
> > > +     if (rc < 0)
> > > +             make_bad_inode(inode);
> > >       /* If ocfs2_read_blocks() got us a new bh, pass it up. */
> > >       if (!rc && !*bh)
> > >               *bh = tmp;
> > > --
> > > 2.43.0
> > >
> > >

Powered by blists - more mailing lists