[PATCH] xfs: allow unlinked symlinks and dirs with zero size

[PATCH] xfs: allow unlinked symlinks and dirs with zero size

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

For a very very long time, inode inactivation has set the inode size to
zero before unmapping the extents associated with the data fork.
Unfortunately, newer commit 3c6f46eacd876 changed the inode verifier to
prohibit zero-length symlinks and directories.  If an inode happens to
get logged in this state and the system crashes before freeing the
inode, log recovery will also fail on the broken inode.

Therefore, allow zero-size symlinks and directories as long as the link
count is zero; nobody will be able to open these files by handle so
there isn't any risk of data exposure.

Fixes: 3c6f46eacd876 ("xfs: sanity check directory inode di_size")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_inode_buf.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 9caf9aa2221d3..afe06cfd6f0cc 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -612,9 +612,23 @@ xfs_dinode_verify(
 	if (mode && xfs_mode_to_ftype(mode) == XFS_DIR3_FT_UNKNOWN)
 		return __this_address;
 
-	/* No zero-length symlinks/dirs. */
-	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
-		return __this_address;
+	/*
+	 * No zero-length symlinks/dirs unless they're unlinked and hence being
+	 * inactivated.
+	 */
+	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0) {
+		if (dip->di_version > 1) {
+			if (dip->di_nlink)
+				return __this_address;
+			else
+				ASSERT(0);
+		} else {
+			if (dip->di_onlink)
+				return __this_address;
+			else
+				ASSERT(0);
+		}
+	}
 
 	fa = xfs_dinode_verify_nrext64(mp, dip);
 	if (fa)

Re: [PATCH] xfs: allow unlinked symlinks and dirs with zero size

[Christoph Hellwig]

On Fri, Jun 07, 2024 at 09:12:17AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> For a very very long time, inode inactivation has set the inode size to
> zero before unmapping the extents associated with the data fork.
> Unfortunately, newer commit 3c6f46eacd876 changed the inode verifier to
> prohibit zero-length symlinks and directories.  If an inode happens to

", newer commit" above reads really odd.  Maybe just drop the "newer "?

> +	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0) {
> +		if (dip->di_version > 1) {
> +			if (dip->di_nlink)
> +				return __this_address;
> +			else
> +				ASSERT(0);
> +		} else {
> +			if (dip->di_onlink)
> +				return __this_address;
> +			else
> +				ASSERT(0);
> +		}

No need for else after a return.

With that fixed:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] xfs: allow unlinked symlinks and dirs with zero size

[Darrick J. Wong]

On Sat, Jun 08, 2024 at 11:33:03PM -0700, Christoph Hellwig wrote:
> On Fri, Jun 07, 2024 at 09:12:17AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > For a very very long time, inode inactivation has set the inode size to
> > zero before unmapping the extents associated with the data fork.
> > Unfortunately, newer commit 3c6f46eacd876 changed the inode verifier to
> > prohibit zero-length symlinks and directories.  If an inode happens to
> 
> ", newer commit" above reads really odd.  Maybe just drop the "newer "?
> 
> > +	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0) {
> > +		if (dip->di_version > 1) {
> > +			if (dip->di_nlink)
> > +				return __this_address;
> > +			else
> > +				ASSERT(0);
> > +		} else {
> > +			if (dip->di_onlink)
> > +				return __this_address;
> > +			else
> > +				ASSERT(0);
> > +		}
> 
> No need for else after a return.
> 
> With that fixed:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

It turns out that even this is still buggy because directories that are
being inactivated (e.g. after repair has replaced the contents) can have
zero isize.  Sooo I'll have a new patch in a day or two.

--D

Re: [PATCH] xfs: allow unlinked symlinks and dirs with zero size

[Christoph Hellwig]

On Mon, Jun 10, 2024 at 02:07:23PM -0700, Darrick J. Wong wrote:
> It turns out that even this is still buggy because directories that are
> being inactivated (e.g. after repair has replaced the contents) can have
> zero isize.  Sooo I'll have a new patch in a day or two.

Isn't that what this patch checks for?  Or do you mean inactivated
with non-zero nlink?

Btw, it might make sense to add a helper or local variable to check
for an unlinked dinode instead of open coding the v1 check twice in
this function.

Re: [PATCH] xfs: allow unlinked symlinks and dirs with zero size

[Darrick J. Wong]

On Mon, Jun 10, 2024 at 09:50:33PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 10, 2024 at 02:07:23PM -0700, Darrick J. Wong wrote:
> > It turns out that even this is still buggy because directories that are
> > being inactivated (e.g. after repair has replaced the contents) can have
> > zero isize.  Sooo I'll have a new patch in a day or two.
> 
> Isn't that what this patch checks for?  Or do you mean inactivated
> with non-zero nlink?

Nah, there's another verifier check elsewhere that also trips when
di-size == 0.

> Btw, it might make sense to add a helper or local variable to check
> for an unlinked dinode instead of open coding the v1 check twice in
> this function.

Maybe?  But the logic is slightly different, isn't it?  I'll have a
look in the morning.

--D