xfs: consolidate local format inode fork verifiers

Create a centralized function to verify local format inode forks, then migrate the existing short format directory verifier to use this framework and add verifiers for the other two things we support (attrs and symlinks).

Obviously this will get broken up into smaller pieces (one patch to refactor/move the existing verifier calls, another one to add the two new verifiers), but what do people think?

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_attr_leaf.c      |   70 ++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr_leaf.h      |    1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |   10 -----
 fs/xfs/libxfs/xfs_shared.h         |    1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |   29 +++++++++++++++
 fs/xfs/xfs_icache.c                |   46 ++++++++++++++++++++++++
 fs/xfs/xfs_icache.h                |    1 +
 fs/xfs/xfs_inode.c                 |    6 +--
 8 files changed, 150 insertions(+), 14 deletions(-)

fs/xfs/libxfs/xfs_attr_leaf.c +13 -0

index c6c15e5..43b881e 100644
@@ -871,6 +871,76 @@ xfs_attr_shortform_allfit(
 	return xfs_attr_shortform_bytesfit(dp, bytes);
 }
 
+/* Verify the consistency of an inline attribute fork. */
+int
+xfs_attr_shortform_verify(
+	struct xfs_inode		*ip)
+{
+	struct xfs_attr_shortform	*sfp;
+	struct xfs_attr_sf_entry	*sfep;
+	struct xfs_attr_sf_entry	*next_sfep;
+	char				*endp;
+	struct xfs_ifork		*ifp;
+	int				i;
+	int				size;
+

fs/xfs/libxfs/xfs_attr_leaf.c +15 -0

+	endp = (char *)sfp + size;
+
+	/* Check all reported entries */
+	sfep = &sfp->list[0];
+	for (i = 0; i < sfp->hdr.count; i++) {
+		/*
+		 * struct xfs_attr_sf_entry has a variable length.
+		 * Check the fixed-offset parts of the structure are
+		 * within the data buffer.
+		 */
+		if (((char *)sfep + sizeof(*sfep)) >= endp)
+			return -EFSCORRUPTED;
+
+		/* Don't allow names with known bad length. */
+		if (sfep->namelen == 0)

fs/xfs/libxfs/xfs_symlink_remote.c +16 -0

index c484877..96e2957 100644
@@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
 	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
 					ifp->if_bytes - 1);
 }
+
+/* Verify the consistency of an inline symlink. */
+int
+xfs_symlink_sf_verify(
+	struct xfs_inode	*ip)
+{
+	char			*sfp;
+	char			*endp;
+	struct xfs_ifork	*ifp;
+	int			size;
+
+	ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	sfp = (char *)ifp->if_u1.if_data;
+	size = ifp->if_bytes;
+	endp = sfp + size;

Quoted 51 lines · Darrick J. Wong

Create a centralized function to verify local format inode forks, then migrate the existing short format directory verifier to use this framework and add verifiers for the other two things we support (attrs and symlinks).

Obviously this will get broken up into smaller pieces (one patch to refactor/move the existing verifier calls, another one to add the two new verifiers), but what do people think?

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_attr_leaf.c      |   70 ++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr_leaf.h      |    1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |   10 -----
 fs/xfs/libxfs/xfs_shared.h         |    1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |   29 +++++++++++++++
 fs/xfs/xfs_icache.c                |   46 ++++++++++++++++++++++++
 fs/xfs/xfs_icache.h                |    1 +
 fs/xfs/xfs_inode.c                 |    6 +--
 8 files changed, 150 insertions(+), 14 deletions(-)

fs/xfs/libxfs/xfs_attr_leaf.c +22 -0

index c6c15e5..43b881e 100644
@@ -871,6 +871,76 @@ xfs_attr_shortform_allfit(
 	return xfs_attr_shortform_bytesfit(dp, bytes);
 }
 
+/* Verify the consistency of an inline attribute fork. */
+int
+xfs_attr_shortform_verify(
+	struct xfs_inode		*ip)
+{
+	struct xfs_attr_shortform	*sfp;
+	struct xfs_attr_sf_entry	*sfep;
+	struct xfs_attr_sf_entry	*next_sfep;
+	char				*endp;
+	struct xfs_ifork		*ifp;
+	int				i;
+	int				size;
+
+	ASSERT(ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL);
+	ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
+	sfp = (struct xfs_attr_shortform *)ifp->if_u1.if_data;
+	size = ifp->if_bytes;
+
+	/*
+	 * Give up if the attribute is way too short.
+	 */
+	if (size < sizeof (struct xfs_attr_sf_hdr))

fs/xfs/libxfs/xfs_attr_leaf.c xfs_attr_shortform_allfit( · line 896

Quoted 1 line

fs/xfs/libxfs/xfs_attr_leaf.c +1 -0

+		return -EFSCORRUPTED;

This seems to already be checked in xfs_iformat_fork() for attr forks. There are also inconsistent checks between data/attr forks whether size is valid prior formatting the fork, so there's more cleanup needed there, and if we are checking attr/dir validity in these functions, we shouldn't be peeking inside the fork in xfs_iformat_fork().

Quoted 15 lines

fs/xfs/libxfs/xfs_attr_leaf.c +15 -0

+	endp = (char *)sfp + size;
+
+	/* Check all reported entries */
+	sfep = &sfp->list[0];
+	for (i = 0; i < sfp->hdr.count; i++) {
+		/*
+		 * struct xfs_attr_sf_entry has a variable length.
+		 * Check the fixed-offset parts of the structure are
+		 * within the data buffer.
+		 */
+		if (((char *)sfep + sizeof(*sfep)) >= endp)
+			return -EFSCORRUPTED;
+
+		/* Don't allow names with known bad length. */
+		if (sfep->namelen == 0)

fs/xfs/libxfs/xfs_attr_leaf.c xfs_attr_shortform_allfit( · line 922

Quoted 10 lines

fs/xfs/libxfs/xfs_attr_leaf.c +10 -0

+			return -EFSCORRUPTED;
+
+		/*
+		 * Check that the variable-length part of the structure is
+		 * within the data buffer.  The next entry starts after the
+		 * name component, so nextentry is an acceptable test.
+		 */
+		next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep);
+		if (endp < (char *)next_sfep)
+			return -EFSCORRUPTED;

Can we make the "past endp" checks consistent in variable order and operators? i.e

		if ((char *)next_sfep >= endp)

[snip]

Quoted 24 lines

fs/xfs/libxfs/xfs_symlink_remote.c +16 -0

index c484877..96e2957 100644
@@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
 	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
 					ifp->if_bytes - 1);
 }
+
+/* Verify the consistency of an inline symlink. */
+int
+xfs_symlink_sf_verify(
+	struct xfs_inode	*ip)
+{
+	char			*sfp;
+	char			*endp;
+	struct xfs_ifork	*ifp;
+	int			size;
+
+	ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	sfp = (char *)ifp->if_u1.if_data;
+	size = ifp->if_bytes;
+	endp = sfp + size;

fs/xfs/libxfs/xfs_symlink_remote.c xfs_symlink_local_to_remote( · line 235

Quoted 10 lines

fs/xfs/libxfs/xfs_symlink_remote.c +10 -0

+
+	/* No negative sizes or overly long symlink targets. */
+	if (size < 0 || size > XFS_SYMLINK_MAXLEN)
+		return -EFSCORRUPTED;
+
+	/* No NULLs in the target either. */
+	for (; sfp < endp; sfp++) {
+		if (*sfp == 0)
+			return -EFSCORRUPTED;
+	}

Hmmm - do we need to check that the symlink has been correctly null terminated in memory (i.e. that *endp == 0) because the VFS requires this and we add the zero termination before checking fork contents validity?

fs/xfs/libxfs/xfs_symlink_remote.c xfs_symlink_local_to_remote( · line 238

Quoted 3 lines

fs/xfs/libxfs/xfs_symlink_remote.c +3 -0

+/* Check inline fork contents. */
+int
+xfs_iget_verify_forks(

*_verify_inline_content()?

fs/xfs/libxfs/xfs_symlink_remote.c

Quoted 3 lines

fs/xfs/libxfs/xfs_symlink_remote.c +3 -0

+	struct xfs_inode		*ip)
+{
+	int				error;

Too much indenting :P

fs/xfs/libxfs/xfs_symlink_remote.c

Quoted 4 lines

fs/xfs/libxfs/xfs_symlink_remote.c +4 -0

+
+	/* Check the attribute fork if there is one. */
+	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
+	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {

If there is no attr fork, then ip->i_d.di_aformat should be set to XFS_DINODE_FMT_EXTENTS. Hence we can just do the same check as for the data fork....

OTOH, having a value of XFS_DINODE_FMT_LOCAL in di_aformat without an attr fork indicates corruption, so perhaps we need to catch that

here as it's not checked in xfs_ifork_format() or xfs_iflush_int(). Indeed, there are partial attr/data fork format/size checks in xfs_ifork_format() and xfs_iflush_int(), but we don't do comprehensive checks in either place. Maybe they should all be moved inside this function and expanded to check that all the fork format information is valid?

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Dave Chinner

fs/xfs/xfs_icache.c xfs_iget_cache_hit( · line 466

Quoted 4 lines · Darrick J. Wong

fs/xfs/xfs_icache.c +4 -0

+
+	/* Check the attribute fork if there is one. */
+	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
+	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {

If there is no attr fork, then ip->i_d.di_aformat should be set to XFS_DINODE_FMT_EXTENTS. Hence we can just do the same check as for the data fork....

Quoted 2 lines

OTOH, having a value of XFS_DINODE_FMT_LOCAL in di_aformat without an attr fork indicates corruption, so perhaps we need to catch that

Hm, the documentation ought to reflect that. :)

Quoted 6 lines

here as it's not checked in xfs_ifork_format() or xfs_iflush_int(). Indeed, there are partial attr/data fork format/size checks in xfs_ifork_format() and xfs_iflush_int(), but we don't do comprehensive checks in either place. Maybe they should all be moved inside this function and expanded to check that all the fork format information is valid?

Yes, this could get cleaned up this way... What if we make _iformat_fork check that the sizes requested aren't insane, allocate memory, and load contents from the dinode; then we later use _verify_ifork_content to pick all the bugs out and destroy the inode if we finds any.

Hm. The bmbt root actually needs numrecs read out of the header. The sf attr header has the total size, though we're never going to need more than (inodesize - forkoff) bytes for it. The bmdr thing could complicate this.

(Maybe I'll sleep on this...)

Also, I thought di_forkoff was multiplied by 8 to calculate the distance of the attr fork from the data fork? If that's the case, then isn't this verifier wrong?

    if (unlikely(dip->di_forkoff > ip->i_mount->m_sb.sb_inodesize)) {

<rambling off topic now>

While we're on the subject of verifiers, Eric Sandeen has been wishing that we could make it easier to figure out which buffer verifier test failed, and it would seem that the XFS_CORRUPTION_ERROR macro is used to highlight bad inode fork contents. Perhaps we should create a similar macro that we could use to log exactly which buffer verifier test failed?

Quoted 2 lines · Darrick J. Wong

Quoted 1 line · Dave Chinner

On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:

Quoted 8 lines

<rambling off topic now>

While we're on the subject of verifiers, Eric Sandeen has been wishing that we could make it easier to figure out which buffer verifier test failed, and it would seem that the XFS_CORRUPTION_ERROR macro is used to highlight bad inode fork contents. Perhaps we should create a similar macro that we could use to log exactly which buffer verifier test failed?

I don't want to put some shouty macro on every second line of a verifier. Think differently - we currently return a true/false from the internal verifier functions to trigger a call to xfs_verifier_error(). How about they return __line__ on error and 0 on success and then pass that returned value into xfs_verifier_error() and add that to the error output?

That tells us which check failed without adding more code to every single verifier check - use the compiler to give us what we need without any additional code, maintenance or runtime overhead. All we need to know is the kernel version so we can translate the line number to a failed check...

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Dave Chinner

Quoted 2 lines · Darrick J. Wong

Quoted 1 line · Dave Chinner

On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:

...

Quoted 16 lines

Quoted 8 lines

<rambling off topic now>

While we're on the subject of verifiers, Eric Sandeen has been wishing that we could make it easier to figure out which buffer verifier test failed, and it would seem that the XFS_CORRUPTION_ERROR macro is used to highlight bad inode fork contents. Perhaps we should create a similar macro that we could use to log exactly which buffer verifier test failed?

I don't want to put some shouty macro on every second line of a verifier. Think differently - we currently return a true/false from the internal verifier functions to trigger a call to xfs_verifier_error(). How about they return __line__ on error and 0 on success and then pass that returned value into xfs_verifier_error() and add that to the error output?

Quoted 6 lines

That tells us which check failed without adding more code to every single verifier check - use the compiler to give us what we need without any additional code, maintenance or runtime overhead. All we need to know is the kernel version so we can translate the line number to a failed check...

I think the ideal situation is the verifier error prints the check that failed, similar to an assert failure.

I'm not aware of any way to do
that without a macro,

but I'm also not against crafting a new, verifier specific one to accomplish that. Technically, it doesn't have to be shouty :), but IMO, the diagnostic/usability benefit outweighs the aesthetic cost.

Beyond that, I'm not against dumping a line number but it would seem kind of unusual to dump a line number without at least a filename. FWIW,

the generic verifier error reporting function also dumps an instruction address for where the report is generated:

XFS (...): Metadata corruption detected at xfs_symlink_read_verify+0xcd/0x100 [xfs], xfs_symlink block 0x58

We obviously want to have information about which verifier failed, but I'm not sure we need the actual address of the xfs_verifier_error() caller. It would be nice if we could replace (the address, not necessarily the function name) that with, or add to it, an address that refers to the particular check that failed.

Quoted 28 lines · Brian Foster

Quoted 3 lines · Dave Chinner

Quoted 2 lines · Darrick J. Wong

Quoted 1 line · Dave Chinner

On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:

...

Quoted 16 lines

Quoted 8 lines

<rambling off topic now>

While we're on the subject of verifiers, Eric Sandeen has been wishing that we could make it easier to figure out which buffer verifier test failed, and it would seem that the XFS_CORRUPTION_ERROR macro is used to highlight bad inode fork contents. Perhaps we should create a similar macro that we could use to log exactly which buffer verifier test failed?

I don't want to put some shouty macro on every second line of a verifier. Think differently - we currently return a true/false from the internal verifier functions to trigger a call to xfs_verifier_error(). How about they return __line__ on error and 0 on success and then pass that returned value into xfs_verifier_error() and add that to the error output?

Quoted 6 lines

That tells us which check failed without adding more code to every single verifier check - use the compiler to give us what we need without any additional code, maintenance or runtime overhead. All we need to know is the kernel version so we can translate the line number to a failed check...

Quoted 2 lines

I think the ideal situation is the verifier error prints the check that failed, similar to an assert failure.

Well, that comes from a macro that feeds the assert failure message __func__ and __line__.

Quoted 2 lines

I'm not aware of any way to do
that without a macro,

I just outlined how to do it above.

Quoted 4 lines

but I'm also not against crafting a new, verifier specific one to accomplish that. Technically, it doesn't have to be shouty :), but IMO, the diagnostic/usability benefit outweighs the aesthetic cost.

I disagree - there are so many verifier checks (and we only grow more as time goes on) so whatever we do needs them to be easily maintainable and not compromise the readabilty of the verifer code.

Quoted 2 lines

Beyond that, I'm not against dumping a line number but it would seem kind of unusual to dump a line number without at least a filename. FWIW,

We don't really need the filename because we have the name of the verifier that failed in the ops structure. Hence we can print a {verfier name, line number} tuple which is effectively the same as {filename, line number}.

Quoted 10 lines

the generic verifier error reporting function also dumps an instruction address for where the report is generated:

XFS (...): Metadata corruption detected at xfs_symlink_read_verify+0xcd/0x100 [xfs], xfs_symlink block 0x58

We obviously want to have information about which verifier failed, but I'm not sure we need the actual address of the xfs_verifier_error() caller. It would be nice if we could replace (the address, not necessarily the function name) that with, or add to it, an address that refers to the particular check that failed.

Yes, that's exactly what I'm proposing we do. What we really need to know is

1. the block that was corrupted (from bp)
2. the verifier that detected the corruption (from bp)
3. the IO type (read/write from bp)
4. the verifier check that failed (returned from verifier)

We already have 1-3, but we don't have 4. We need to replace the replace __return_address used in the error message with __line__ or __THIS_IP__ that is returned from the if() branch that failed and from that we can then easily track the cause of the failure back to the source.

Returning __line__ or __THIS_IP__ from the verifier doesn't require new macros, or really any significant code change as most verifiers arelady return a true/false. All we need to do is plumb it into xfs_verifier_error().

With that extra info we can output a slightly different message, say:

XFS (...): Metadata corruption detected during read of block 0xx58 by xfs_symlink verifier, line 132

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 7 lines · Dave Chinner

Quoted 5 lines · Brian Foster

On Fri, Jul 21, 2017 at 08:49:34AM +1000, Dave Chinner wrote:
but I'm also not against crafting a new, verifier specific one to accomplish that. Technically, it doesn't have to be shouty :), but IMO, the diagnostic/usability benefit outweighs the aesthetic cost.

Quoted 5 lines

I disagree - there are so many verifier checks (and we only grow more as time goes on) so whatever we do needs them to be easily maintainable and not compromise the readabilty of the verifer code.

I agree. ;) But I disagree that using a macro somehow automatically negatively affects the readability of verifers. Anybody who can make sense of this:

        if (!xfs_sb_version_hascrc(&mp->m_sb))
                return __line__;
        if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
                return __line__;
        if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
                return __line__;

... could just as easily make sense of something like this:

	xfs_verify_assert(xfs_sb_version_hascrc(&mp->m_sb));
	xfs_verify_assert(dsl->sl_magic == cpu_to_be32(XFS_SYMLINK_MAGIC));
	xfs_verify_assert(uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid));

There is a simple solution to this problem: just include all of the readily accessible state information in the error report. If the messages above looked something like the following:

XFS (...): Metadata corruption detected at xfs_symlink_read_verify [xfs], xfs_symlink block 0x58 XFS (...): Read verifier failed: xfs_log_check_lsn(mp, be64_to_cpu(dsl->sl_lsn)), xfs_symlink_remote.c:114

... we wouldn't have to decode what the error means for every possible kernel. The error report is self contained like most of the other errors/assert reports in XFS and throughout the kernel.

Quoted 25 lines · Brian Foster

Quoted 7 lines · Dave Chinner

Quoted 5 lines · Brian Foster

On Fri, Jul 21, 2017 at 08:49:34AM +1000, Dave Chinner wrote:
but I'm also not against crafting a new, verifier specific one to accomplish that. Technically, it doesn't have to be shouty :), but IMO, the diagnostic/usability benefit outweighs the aesthetic cost.

Quoted 5 lines

I disagree - there are so many verifier checks (and we only grow more as time goes on) so whatever we do needs them to be easily maintainable and not compromise the readabilty of the verifer code.

I agree. ;) But I disagree that using a macro somehow automatically negatively affects the readability of verifers. Anybody who can make sense of this:

        if (!xfs_sb_version_hascrc(&mp->m_sb))
                return __line__;
        if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
                return __line__;
        if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
                return __line__;

Quoted 5 lines

... could just as easily make sense of something like this:

	xfs_verify_assert(xfs_sb_version_hascrc(&mp->m_sb));
	xfs_verify_assert(dsl->sl_magic == cpu_to_be32(XFS_SYMLINK_MAGIC));
	xfs_verify_assert(uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid));

Still needs branches to return on error so the verifier code functions correctly. So it simply isn't as clean as your example suggests. And, no hiding goto/return/errno variables inside a macro is not an option - that makes for horrible code and we should not repeat those past mistakes we inherited from the old Irix codebase.

And we need to be really careful here - making the code more complex for pretty errors blows out the size of the code. That means it runs slower, even though we never execute most of the pretty error print stuff. This is an important consideration as verifiers are a performance critical fast path, both in the kernel and in userspace.

Quoted 10 lines

There is a simple solution to this problem: just include all of the readily accessible state information in the error report. If the messages above looked something like the following:

XFS (...): Metadata corruption detected at xfs_symlink_read_verify [xfs], xfs_symlink block 0x58 XFS (...): Read verifier failed: xfs_log_check_lsn(mp, be64_to_cpu(dsl->sl_lsn)), xfs_symlink_remote.c:114

... we wouldn't have to decode what the error means for every possible kernel. The error report is self contained like most of the other errors/assert reports in XFS and throughout the kernel.

Just like an assert/warning, we've still got to go match it to the exact kernel source tree to determine the context in which the message was emitted and what it actually means. Making it pretty doesn't change the amount of work a developer needs to do to classify or analyse errors in log files, all it does is change the amount of overhead needed to generate the error message,

Really, verifiers are performance critical fast paths, so the code that runs inside them needs to be implemented with this in mind. I'm not against making messages pretty, but let's not do it based on the on the premise that pretty error messages are more important than runtime performance when there are no errors...

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

Quoted 1 line · Brian Foster

On Fri, Jul 21, 2017 at 08:49:34AM +1000, Dave Chinner wrote:

[1] A simple test that we can do at the moment is to compare a production xfs.ko with one with asserts enabled. My local for-next branch builds a module of 40104000 bytes without XFS_WARN and 40540208 bytes with XFS_WARN enabled. That makes for a difference of ~425k and roughly a 1% code size increase (accounting for ~1750 asserts and supporting code).

[2] We may not want to use ASSERT() here, but perhaps define a new variant to omit the stack trace and otherwise customize to a verifier report.

Quoted 11 lines

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 32 lines · Dave Chinner

Quoted 25 lines · Brian Foster

Quoted 7 lines · Dave Chinner

Quoted 5 lines · Brian Foster

On Fri, Jul 21, 2017 at 08:49:34AM +1000, Dave Chinner wrote:
but I'm also not against crafting a new, verifier specific one to accomplish that. Technically, it doesn't have to be shouty :), but IMO, the diagnostic/usability benefit outweighs the aesthetic cost.

Quoted 5 lines

I disagree - there are so many verifier checks (and we only grow more as time goes on) so whatever we do needs them to be easily maintainable and not compromise the readabilty of the verifer code.

I agree. ;) But I disagree that using a macro somehow automatically negatively affects the readability of verifers. Anybody who can make sense of this:

        if (!xfs_sb_version_hascrc(&mp->m_sb))
                return __line__;
        if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
                return __line__;
        if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
                return __line__;

Quoted 5 lines

... could just as easily make sense of something like this:

	xfs_verify_assert(xfs_sb_version_hascrc(&mp->m_sb));
	xfs_verify_assert(dsl->sl_magic == cpu_to_be32(XFS_SYMLINK_MAGIC));
	xfs_verify_assert(uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid));

Quoted 7 lines

Still needs branches to return on error so the verifier code functions correctly. So it simply isn't as clean as your example suggests. And, no hiding goto/return/errno variables inside a macro is not an option - that makes for horrible code and we should not repeat those past mistakes we inherited from the old Irix codebase.

Clearly the example implies (and the supporting text explains) that the macro would lead to function exit.

I don't really see the problem with doing that. Again, we already do that all over the place. If we don't want to alter function execution via the macro, then have it return an expression that resolves to false (or an error code or some such) and leave the function execution logic in function context. It doesn't really matter to me either way.

Quoted 5 lines

And we need to be really careful here - making the code more complex for pretty errors blows out the size of the code. That means it runs slower, even though we never execute most of the pretty error print stuff. This is an important consideration as verifiers are a performance critical fast path, both in the kernel and in userspace.

I guess we'd increase the size of the code for strings that represent the associated checks and whatever extra the macro code may end up duplicating. Once an error is detected, I think much of the latter could be packaged into a noinline helper. It's not immediately clear to me what we're looking at for a size increase, so I don't think "blows out" is a fair characterization without some actual data [1]. I do think this is a fair/valid concern however, we'd just need to try and experiment and see how much it costs to convert over a verifier or two, for example.

And I'm not sure how you make the leap from there to an automatic performance degradation. I also agree that the verifiers are performance critical and wouldn't suggest taking an approach if I thought we'd measurably affect performance. As it is, I'd expect that additional overhead of storing error context (hell, we could always just dump it from where the problem was originally detected rather than pass it to the generic verifier report function) would essentially be nil up until a verification check fails.

Would you consider such an approach if we could demonstrate a reasonable code size tradeoff and that runtime performance is not affected? If you simply hate macros then I guess there isn't much we can do but agree to disagree (and I'm not against a non-macro approach if one exists, I just don't know what it would be). But otherwise I wouldn't mind running some experiments (unless Darrick wanted to..? he brought this up after all.. :P) if the results will be fairly considered.

Quoted 12 lines

Quoted 11 lines

There is a simple solution to this problem: just include all of the
readily accessible state information in the error report. If the messages above looked something like the following:

XFS (...): Metadata corruption detected at xfs_symlink_read_verify [xfs], xfs_symlink block 0x58 XFS (...): Read verifier failed: xfs_log_check_lsn(mp, be64_to_cpu(dsl->sl_lsn)), xfs_symlink_remote.c:114

... we wouldn't have to decode what the error means for every possible kernel. The error report is self contained like most of the other errors/assert reports in XFS and throughout the kernel.

Quoted 7 lines

Just like an assert/warning, we've still got to go match it to the exact kernel source tree to determine the context in which the message was emitted and what it actually means. Making it pretty doesn't change the amount of work a developer needs to do to classify or analyse errors in log files, all it does is change the amount of overhead needed to generate the error message,

Yes, you're still going to want to ultimately dig down into the code and do complex root cause analysis. Unfortunately, we can't fix that with error reporting. :) I've laid out several situations above as to why I think the "pretty" format makes it significantly easier to make sense of corruption reports that may be more involved than a single instance or a single physical node. A broad assessment can be made from the actual corruption report without the need to decode tens or more cryptic messages. It potentially saves significant time spent doing unnecessary busy work. It also helps reduce unnecessary confusion with general reports that might arise from users on distro kernels, etc., where the line numbers might not be sufficient or easily misaligned.

Of course, once we get into debugging an actual problem, we've kind of moved outside the scope of this discussion...

Quoted 6 lines

Really, verifiers are performance critical fast paths, so the code that runs inside them needs to be implemented with this in mind. I'm not against making messages pretty, but let's not do it based on the on the premise that pretty error messages are more important than runtime performance when there are no errors...

I agree. As noted above, I would not expect any more overhead than the original suggestion to return a line number in the normal runtime case. The difference is primarily what we do in the code between when a verifier check fails and we report the failure.

FWIW, an approach that might be a reasonable compromise is to use the original suggestion to pass along the line number, but also wrap each verifier check in a thinner macro that simply asserts[2] on the verifier expression. The new macro could be ifdef'd out completely in production builds to eliminate code size concerns. Then if we have reports with significant enough corruption issues to warrant it, we collect a metadump and use any local debug kernel to access the fs and decode the errors for us (or ask the user to run a debug kernel). We can expand the verifier macro in the future with production build support if warranted. Thoughts?

Brian

[1] A simple test that we can do at the moment is to compare a production xfs.ko with one with asserts enabled. My local for-next branch builds a module of 40104000 bytes without XFS_WARN and 40540208 bytes with XFS_WARN enabled. That makes for a difference of ~425k and roughly a 1% code size increase (accounting for ~1750 asserts and supporting code).

[2] We may not want to use ASSERT() here, but perhaps define a new variant to omit the stack trace and otherwise customize to a verifier report.

Quoted 12 lines

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 41 lines · Brian Foster

Quoted 39 lines · Dave Chinner

Quoted 25 lines · Brian Foster

Quoted 7 lines · Dave Chinner

Quoted 5 lines · Brian Foster

On Fri, Jul 21, 2017 at 08:49:34AM +1000, Dave Chinner wrote:
but I'm also not against crafting a new, verifier specific one to accomplish that. Technically, it doesn't have to be shouty :), but IMO, the diagnostic/usability benefit outweighs the aesthetic cost.

Quoted 5 lines

I disagree - there are so many verifier checks (and we only grow more as time goes on) so whatever we do needs them to be easily maintainable and not compromise the readabilty of the verifer code.

I agree. ;) But I disagree that using a macro somehow automatically negatively affects the readability of verifers. Anybody who can make sense of this:

        if (!xfs_sb_version_hascrc(&mp->m_sb))
                return __line__;
        if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
                return __line__;
        if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
                return __line__;

Quoted 5 lines

... could just as easily make sense of something like this:

	xfs_verify_assert(xfs_sb_version_hascrc(&mp->m_sb));
	xfs_verify_assert(dsl->sl_magic == cpu_to_be32(XFS_SYMLINK_MAGIC));
	xfs_verify_assert(uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid));

Still needs branches to return on error so the verifier code functions correctly. So it simply isn't as clean as your example suggests. And, no hiding goto/return/errno variables inside a macro is not an option - that makes for horrible code and we should not repeat those past mistakes we inherited from the old Irix codebase.

Quoted 2 lines

Clearly the example implies (and the supporting text explains) that the macro would lead to function exit.

It does? Every time I see an *assert() I assume that execution either continues past it or stops dead in its tracks. The _GOTO macros make that clear by pairing the word goto with a label, though I concede that it feels odd to pass a label as an argument.

Quoted 5 lines

I don't really see the problem with doing that. Again, we already do that all over the place. If we don't want to alter function execution via the macro, then have it return an expression that resolves to false (or an error code or some such) and leave the function execution logic in function context. It doesn't really matter to me either way.

I get that hiding branches inside macros makes it too easy for later readers to be mislead, but if there's one aspect of them that I /do/ like, its that (provided the naming makes it clear that we're branching somewhere) we /can/ use them to ensure that the return values are consistent and avoid screwing up repetitive if tests.

Anyway, I was picturing something like this:

#define XFS_VERIFY_FAIL_UNLESS(expr) do { if (expr) return _THIS_IP_; } while(0)

static unsigned long
xfs_blah_verify(...)

{
	XFS_VERIFY_FAIL_UNLESS(blah1 == GOOD1);
	XFS_VERIFY_FAIL_UNLESS(blah2 == GOOD2);
	XFS_VERIFY_FAIL_UNLESS(blah3 == GOOD3);
	...
	return 0;
}

Granted, with code review to spot inconsistencies maybe the macro really is pointless, and we can just open-code everything. I dunno. To be honest I actually do prefer the macro in this particular case.

static unsigned long
xfs_blah_verify(...)

{
	if (!(blah1 == GOOD1))
		return _THIS_IP_;
	if (!(blah2 == GOOD2))
		return _THIS_IP_;
	if (!(blah3 == GOOD3))
		return _THIS_IP_;
	...
	return 0;
}

I don't think there's much difference in the generated code between opencoded verifier tests and a macro that more or less does the same thing but also helps us to avoid copy-pasta mistakes. TBH I'm more worried about us making a mistake in the verifiers that screw up what we accept coming from / allow to be written to disk than I am about the exact size of each verifier function.

That said, I've warmed to Dave's idea of passing back _THIS_IP_ as a compromise between __line__ (not quite enough info) and throwing (#fs_ok, __func__, __line__) back to callers.

Admittedly, bloating up the scrub module with stringified stuff has made it way quicker for me to figure out what got flagged as a failure and then decide if the check I wrote is incorrect or if we really did find something broken. One more upside to the macro is that we could easily make each verifier test spit out more information if CONFIG_XFS_DEBUG is set. This won't help us with customers encountering production problems, obviously, but I think there's still value.

Quoted 15 lines

Of course, once we get into debugging an actual problem, we've kind of moved outside the scope of this discussion...

Quoted 6 lines

Really, verifiers are performance critical fast paths, so the code that runs inside them needs to be implemented with this in mind. I'm not against making messages pretty, but let's not do it based on the on the premise that pretty error messages are more important than runtime performance when there are no errors...

I agree. As noted above, I would not expect any more overhead than the original suggestion to return a line number in the normal runtime case. The difference is primarily what we do in the code between when a verifier check fails and we report the failure.

Quoted 10 lines

FWIW, an approach that might be a reasonable compromise is to use the original suggestion to pass along the line number, but also wrap each verifier check in a thinner macro that simply asserts[2] on the verifier expression. The new macro could be ifdef'd out completely in production builds to eliminate code size concerns. Then if we have reports with significant enough corruption issues to warrant it, we collect a metadump and use any local debug kernel to access the fs and decode the errors for us (or ask the user to run a debug kernel). We can expand the verifier macro in the future with production build support if warranted. Thoughts?

<shrug> _THIS_IP_ ?

(Sorry for starting to sound like a broken record...)

Quoted 9 lines

Brian

[1] A simple test that we can do at the moment is to compare a production xfs.ko with one with asserts enabled. My local for-next branch builds a module of 40104000 bytes without XFS_WARN and 40540208 bytes with XFS_WARN enabled. That makes for a difference of ~425k and roughly a 1% code size increase (accounting for ~1750 asserts and supporting code).

Quoted 3 lines

[2] We may not want to use ASSERT() here, but perhaps define a new variant to omit the stack trace and otherwise customize to a verifier report.

Speaking of ASSERTs, I was going to propose removing the ASSERT calls from the XFS_WANT_CORRUPTED_* macros since they're used in some of the directory verifiers and we shouldn't really BUG_ON corrupted disk contents. An XFS_ERROR_REPORT follows soon after the ASSERT, so imho I think we could get by with only one round of complaining.

--D

Quoted 12 lines

Quoted 12 lines

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 29 lines · Darrick J. Wong

Quoted 10 lines · Dave Chinner

fs/xfs/xfs_icache.c xfs_iget_cache_hit( · line 466

Quoted 4 lines · Darrick J. Wong

fs/xfs/xfs_icache.c +4 -0

+
+	/* Check the attribute fork if there is one. */
+	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
+	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {

If there is no attr fork, then ip->i_d.di_aformat should be set to XFS_DINODE_FMT_EXTENTS. Hence we can just do the same check as for the data fork....

Quoted 2 lines

OTOH, having a value of XFS_DINODE_FMT_LOCAL in di_aformat without an attr fork indicates corruption, so perhaps we need to catch that

Hm, the documentation ought to reflect that. :)

Quoted 6 lines

here as it's not checked in xfs_ifork_format() or xfs_iflush_int(). Indeed, there are partial attr/data fork format/size checks in xfs_ifork_format() and xfs_iflush_int(), but we don't do comprehensive checks in either place. Maybe they should all be moved inside this function and expanded to check that all the fork format information is valid?

Yes, this could get cleaned up this way... What if we make _iformat_fork check that the sizes requested aren't insane, allocate memory, and load contents from the dinode; then we later use _verify_ifork_content to pick all the bugs out and destroy the inode if we finds any.

Quoted 4 lines

Hm. The bmbt root actually needs numrecs read out of the header. The sf attr header has the total size, though we're never going to need more than (inodesize - forkoff) bytes for it. The bmdr thing could complicate this.

Right, but that's only when the fork format is in XFS_DINODE_FMT_BTREE, so it's not an issue for local format validation.

Quoted 1 line

(Maybe I'll sleep on this...)

Quoted 5 lines

Also, I thought di_forkoff was multiplied by 8 to calculate the distance of the attr fork from the data fork? If that's the case, then isn't this verifier wrong?

    if (unlikely(dip->di_forkoff > ip->i_mount->m_sb.sb_inodesize)) {

Yeah, but rather than "wrong" it's better to think of it as "not exact". It'll catch most errors, but not really small ones. ISTR that quite a few of the initial verifiers I wrote did things like this because it wasn't possible or not clear how to do exact range checks on some values.

In this case, if we want exact checks then we have to consider that the di_forkoff has both a minimum and a maximum valid value - the minimum valid value is dependent on data fork contents, the maximum is inodesize - inode core size....

Quoted 8 lines

<rambling off topic now>

While we're on the subject of verifiers, Eric Sandeen has been wishing that we could make it easier to figure out which buffer verifier test failed, and it would seem that the XFS_CORRUPTION_ERROR macro is used to highlight bad inode fork contents. Perhaps we should create a similar macro that we could use to log exactly which buffer verifier test failed?

I don't want to put some shouty macro on every second line of a verifier. Think differently - we currently return a true/false from the internal verifier functions to trigger a call to xfs_verifier_error(). How about they return __line__ on error and 0 on success and then pass that returned value into xfs_verifier_error() and add that to the error output?

That tells us which check failed without adding more code to every single verifier check - use the compiler to give us what we need without any additional code, maintenance or runtime overhead. All we need to know is the kernel version so we can translate the line number to a failed check...

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Create a centralized function to verify local format inode forks, then migrate the existing short format directory verifier to use this framework and add verifiers for the other two things we support (attrs and symlinks).

Obviously this will get broken up into smaller pieces (one patch to refactor/move the existing verifier calls, another one to add the two new verifiers), but what do people think?

Signed-off-by Darrick J. Wong <[email protected]>

---

 fs/xfs/libxfs/xfs_attr_leaf.c      |   70 ++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr_leaf.h      |    1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |   10 -----
 fs/xfs/libxfs/xfs_shared.h         |    1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |   29 +++++++++++++++
 fs/xfs/xfs_icache.c                |   46 ++++++++++++++++++++++++
 fs/xfs/xfs_icache.h                |    1 +
 fs/xfs/xfs_inode.c                 |    6 +--
 8 files changed, 150 insertions(+), 14 deletions(-)

fs/xfs/libxfs/xfs_inode_fork.c +0 -0

index 0e80f34..5484211 100644
@@ -183,16 +183,6 @@ xfs_iformat_fork(
 	if (error)
 		return error;
 

fs/xfs/libxfs/xfs_inode_fork.c +0 -0

 	if (xfs_is_reflink_inode(ip)) {
 		ASSERT(ip->i_cowfp == NULL);
 		xfs_ifork_init_cow(ip);

fs/xfs/libxfs/xfs_symlink_remote.c +16 -0

index c484877..96e2957 100644
@@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
 	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
 					ifp->if_bytes - 1);
 }
+
+/* Verify the consistency of an inline symlink. */
+int
+xfs_symlink_sf_verify(
+	struct xfs_inode	*ip)
+{
+	char			*sfp;
+	char			*endp;
+	struct xfs_ifork	*ifp;
+	int			size;
+
+	ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	sfp = (char *)ifp->if_u1.if_data;
+	size = ifp->if_bytes;
+	endp = sfp + size;

fs/xfs/libxfs/xfs_symlink_remote.c +3 -0

+
+	return 0;
+}

fs/xfs/xfs_icache.c +46 -0

index 0a9e698..fd1d430 100644
@@ -34,6 +34,11 @@
 #include "xfs_dquot_item.h"
 #include "xfs_dquot.h"
 #include "xfs_reflink.h"
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
+#include "xfs_dir2_priv.h"
+#include "xfs_attr_leaf.h"
+#include "xfs_shared.h"
 
 #include <linux/kthread.h>
 #include <linux/freezer.h>

@@ -449,6 +454,43 @@ xfs_iget_cache_hit(
 	return error;
 }
 
+/* Check inline fork contents. */
+int
+xfs_iget_verify_forks(
+	struct xfs_inode		*ip)
+{
+	int				error;
+
+	/* Check the attribute fork if there is one. */
+	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
+	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+		error = xfs_attr_shortform_verify(ip);
+		if (error) {
+			xfs_alert(ip->i_mount,
+					"%s: bad inode %Lu inline attr fork",
+					__func__, ip->i_ino);
+			return error;
+		}
+	}
+
+	/* Non-local data fork, jump out. */
+	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
+		return 0;
+
+	/* Check the inline data fork if there is one. */
+	if (S_ISDIR(VFS_I(ip)->i_mode))
+		error = xfs_dir2_sf_verify(ip);
+	else if (S_ISLNK(VFS_I(ip)->i_mode))
+		error = xfs_symlink_sf_verify(ip);
+	else
+		error = -EFSCORRUPTED;
+
+	if (error)
+		xfs_alert(ip->i_mount, "%s: bad inode %Lu inline data fork",
+				__func__, ip->i_ino);
+	return error;
+}
+
 
 static int
 xfs_iget_cache_miss(

@@ -473,6 +515,10 @@ xfs_iget_cache_miss(
 	if (error)
 		goto out_destroy;
 
+	error = xfs_iget_verify_forks(ip);
+	if (error)
+		goto out_destroy;
+
 	trace_xfs_iget_miss(ip);
 
 	if ((VFS_I(ip)->i_mode == 0) && !(flags & XFS_IGET_CREATE)) {

fs/xfs/xfs_icache.h +1 -0

index bff4d85..abf1cff 100644
@@ -129,5 +129,6 @@ xfs_fs_eofblocks_from_user(
 
 int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
 				  xfs_ino_t ino, bool *inuse);
+int xfs_iget_verify_forks(struct xfs_inode *ip);
 
 #endif

fs/xfs/xfs_inode.c +2 -5

index ceef77c..7ca8336 100644
@@ -3547,10 +3547,8 @@ xfs_iflush_int(
 	if (ip->i_d.di_version < 3)
 		ip->i_d.di_flushiter++;
 
-	/* Check the inline directory data. */
-	if (S_ISDIR(VFS_I(ip)->i_mode) &&
-	    ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
-	    xfs_dir2_sf_verify(ip))
+	/* Check the inline fork data. */
+	if (xfs_iget_verify_forks(ip))
 		goto corrupt_out;
 
 	/*
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Darrick J. Wong

Create a centralized function to verify local format inode forks, then migrate the existing short format directory verifier to use this framework and add verifiers for the other two things we support (attrs and symlinks).

Obviously this will get broken up into smaller pieces (one patch to refactor/move the existing verifier calls, another one to add the two new verifiers), but what do people think?

Signed-off-by Darrick J. Wong <[email protected]>

---

I haven't combed through the details of the verifiers, but just a few very quick thoughts..

Quoted 10 lines

 fs/xfs/libxfs/xfs_attr_leaf.c      |   70 ++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr_leaf.h      |    1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |   10 -----
 fs/xfs/libxfs/xfs_shared.h         |    1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |   29 +++++++++++++++
 fs/xfs/xfs_icache.c                |   46 ++++++++++++++++++++++++
 fs/xfs/xfs_icache.h                |    1 +
 fs/xfs/xfs_inode.c                 |    6 +--
 8 files changed, 150 insertions(+), 14 deletions(-)

...

Quoted 8 lines

fs/xfs/libxfs/xfs_inode_fork.c +0 -0

index 0e80f34..5484211 100644
@@ -183,16 +183,6 @@ xfs_iformat_fork(
 	if (error)
 		return error;
 

fs/xfs/libxfs/xfs_inode_fork.c xfs_iformat_fork( · line 195

Quoted 10 lines

fs/xfs/libxfs/xfs_inode_fork.c +0 -10

-	/* Check inline dir contents. */
-	if (S_ISDIR(VFS_I(ip)->i_mode) &&
-	    dip->di_format == XFS_DINODE_FMT_LOCAL) {
-		error = xfs_dir2_sf_verify(ip);
-		if (error) {
-			xfs_idestroy_fork(ip, XFS_DATA_FORK);
-			return error;
-		}
-	}
-

What's the purpose of moving this? Just to accommodate userspace?

As it is, it looks like this leaves a coverage gap in a log recovery case where xfs_iget() is not used (xfs_recover_inode_owner_change()).

Also, I think this is best split up into two or three smaller patches. E.g., one to move/rename the existing mechanism (with justification as to why), one or more to add the additional verifiers for the other formats.

Quoted 3 lines

fs/xfs/libxfs/xfs_inode_fork.c +0 -0

 	if (xfs_is_reflink_inode(ip)) {
 		ASSERT(ip->i_cowfp == NULL);
 		xfs_ifork_init_cow(ip);

...

Quoted 28 lines

fs/xfs/libxfs/xfs_symlink_remote.c +20 -0

index c484877..96e2957 100644
@@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
 	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
 					ifp->if_bytes - 1);
 }
+
+/* Verify the consistency of an inline symlink. */
+int
+xfs_symlink_sf_verify(
+	struct xfs_inode	*ip)
+{
+	char			*sfp;
+	char			*endp;
+	struct xfs_ifork	*ifp;
+	int			size;
+
+	ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	sfp = (char *)ifp->if_u1.if_data;
+	size = ifp->if_bytes;
+	endp = sfp + size;
+
+	/* No negative sizes or overly long symlink targets. */
+	if (size < 0 || size > XFS_SYMLINK_MAXLEN)
+		return -EFSCORRUPTED;

fs/xfs/libxfs/xfs_symlink_remote.c xfs_symlink_local_to_remote( · line 235

Quoted 6 lines

fs/xfs/libxfs/xfs_symlink_remote.c +6 -0

+
+	/* No NULLs in the target either. */
+	for (; sfp < endp; sfp++) {
+		if (*sfp == 0)
+			return -EFSCORRUPTED;
+	}

	memchr() ?

Brian

Quoted 103 lines

fs/xfs/libxfs/xfs_symlink_remote.c +3 -0

+
+	return 0;
+}

fs/xfs/xfs_icache.c +46 -0

index 0a9e698..fd1d430 100644
@@ -34,6 +34,11 @@
 #include "xfs_dquot_item.h"
 #include "xfs_dquot.h"
 #include "xfs_reflink.h"
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
+#include "xfs_dir2_priv.h"
+#include "xfs_attr_leaf.h"
+#include "xfs_shared.h"
 
 #include <linux/kthread.h>
 #include <linux/freezer.h>

@@ -449,6 +454,43 @@ xfs_iget_cache_hit(
 	return error;
 }
 
+/* Check inline fork contents. */
+int
+xfs_iget_verify_forks(
+	struct xfs_inode		*ip)
+{
+	int				error;
+
+	/* Check the attribute fork if there is one. */
+	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
+	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+		error = xfs_attr_shortform_verify(ip);
+		if (error) {
+			xfs_alert(ip->i_mount,
+					"%s: bad inode %Lu inline attr fork",
+					__func__, ip->i_ino);
+			return error;
+		}
+	}
+
+	/* Non-local data fork, jump out. */
+	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
+		return 0;
+
+	/* Check the inline data fork if there is one. */
+	if (S_ISDIR(VFS_I(ip)->i_mode))
+		error = xfs_dir2_sf_verify(ip);
+	else if (S_ISLNK(VFS_I(ip)->i_mode))
+		error = xfs_symlink_sf_verify(ip);
+	else
+		error = -EFSCORRUPTED;
+
+	if (error)
+		xfs_alert(ip->i_mount, "%s: bad inode %Lu inline data fork",
+				__func__, ip->i_ino);
+	return error;
+}
+
 
 static int
 xfs_iget_cache_miss(

@@ -473,6 +515,10 @@ xfs_iget_cache_miss(
 	if (error)
 		goto out_destroy;
 
+	error = xfs_iget_verify_forks(ip);
+	if (error)
+		goto out_destroy;
+
 	trace_xfs_iget_miss(ip);
 
 	if ((VFS_I(ip)->i_mode == 0) && !(flags & XFS_IGET_CREATE)) {

fs/xfs/xfs_icache.h +1 -0

index bff4d85..abf1cff 100644
@@ -129,5 +129,6 @@ xfs_fs_eofblocks_from_user(
 
 int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
 				  xfs_ino_t ino, bool *inuse);
+int xfs_iget_verify_forks(struct xfs_inode *ip);
 
 #endif

fs/xfs/xfs_inode.c +2 -5

index ceef77c..7ca8336 100644
@@ -3547,10 +3547,8 @@ xfs_iflush_int(
 	if (ip->i_d.di_version < 3)
 		ip->i_d.di_flushiter++;
 
-	/* Check the inline directory data. */
-	if (S_ISDIR(VFS_I(ip)->i_mode) &&
-	    ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
-	    xfs_dir2_sf_verify(ip))
+	/* Check the inline fork data. */
+	if (xfs_iget_verify_forks(ip))
 		goto corrupt_out;
 
 	/*
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 35 lines · Brian Foster

Quoted 11 lines · Darrick J. Wong

Create a centralized function to verify local format inode forks, then migrate the existing short format directory verifier to use this framework and add verifiers for the other two things we support (attrs and symlinks).

Obviously this will get broken up into smaller pieces (one patch to refactor/move the existing verifier calls, another one to add the two new verifiers), but what do people think?

Signed-off-by Darrick J. Wong <[email protected]>

---

I haven't combed through the details of the verifiers, but just a few very quick thoughts..

Quoted 10 lines

 fs/xfs/libxfs/xfs_attr_leaf.c      |   70 ++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr_leaf.h      |    1 +
 fs/xfs/libxfs/xfs_inode_fork.c     |   10 -----
 fs/xfs/libxfs/xfs_shared.h         |    1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |   29 +++++++++++++++
 fs/xfs/xfs_icache.c                |   46 ++++++++++++++++++++++++
 fs/xfs/xfs_icache.h                |    1 +
 fs/xfs/xfs_inode.c                 |    6 +--
 8 files changed, 150 insertions(+), 14 deletions(-)

...

Quoted 8 lines

fs/xfs/libxfs/xfs_inode_fork.c +0 -0

index 0e80f34..5484211 100644
@@ -183,16 +183,6 @@ xfs_iformat_fork(
 	if (error)
 		return error;
 

Quoted 12 lines

fs/xfs/libxfs/xfs_inode_fork.c xfs_iformat_fork( · line 195

Quoted 10 lines

fs/xfs/libxfs/xfs_inode_fork.c +0 -10

-	/* Check inline dir contents. */
-	if (S_ISDIR(VFS_I(ip)->i_mode) &&
-	    dip->di_format == XFS_DINODE_FMT_LOCAL) {
-		error = xfs_dir2_sf_verify(ip);
-		if (error) {
-			xfs_idestroy_fork(ip, XFS_DATA_FORK);
-			return error;
-		}
-	}
-

What's the purpose of moving this? Just to accommodate userspace?

Yes, it's part of letting repair supply its own ifork verifiers so that phase 6 can libxfs_iget to inspect and correct shortform directories.

Quoted 2 lines

As it is, it looks like this leaves a coverage gap in a log recovery case where xfs_iget() is not used (xfs_recover_inode_owner_change()).

Ugh, I didn't even notice that.

As it stands today, xfs_recover_inode_owner_change also won't notice corrupt attr forks or inline symlinks. Seeing as we're (theoretically) only changing the owner fields in v5 bmbt tree blocks this might not have any practical consequence. At that point we've already replayed everything else in that inode that was dirty and are about to write things out to disk, so everything has to be correct.

Quoted 4 lines

Also, I think this is best split up into two or three smaller patches. E.g., one to move/rename the existing mechanism (with justification as to why), one or more to add the additional verifiers for the other formats.

(I mentioned that I'd do that before sending a real series...)

Quoted 32 lines

Quoted 3 lines

fs/xfs/libxfs/xfs_inode_fork.c +0 -0

 	if (xfs_is_reflink_inode(ip)) {
 		ASSERT(ip->i_cowfp == NULL);
 		xfs_ifork_init_cow(ip);

...

Quoted 28 lines

fs/xfs/libxfs/xfs_symlink_remote.c +20 -0

index c484877..96e2957 100644
@@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
 	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
 					ifp->if_bytes - 1);
 }
+
+/* Verify the consistency of an inline symlink. */
+int
+xfs_symlink_sf_verify(
+	struct xfs_inode	*ip)
+{
+	char			*sfp;
+	char			*endp;
+	struct xfs_ifork	*ifp;
+	int			size;
+
+	ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	sfp = (char *)ifp->if_u1.if_data;
+	size = ifp->if_bytes;
+	endp = sfp + size;
+
+	/* No negative sizes or overly long symlink targets. */
+	if (size < 0 || size > XFS_SYMLINK_MAXLEN)
+		return -EFSCORRUPTED;

Quoted 8 lines

fs/xfs/libxfs/xfs_symlink_remote.c xfs_symlink_local_to_remote( · line 235

Quoted 6 lines

fs/xfs/libxfs/xfs_symlink_remote.c +6 -0

+
+	/* No NULLs in the target either. */
+	for (; sfp < endp; sfp++) {
+		if (*sfp == 0)
+			return -EFSCORRUPTED;
+	}

	memchr() ?

<nod> Will do. I should also check the padding fields too...

--D

Quoted 103 lines

Brian

Quoted 101 lines

fs/xfs/libxfs/xfs_symlink_remote.c +3 -0

+
+	return 0;
+}

fs/xfs/xfs_icache.c +46 -0

index 0a9e698..fd1d430 100644
@@ -34,6 +34,11 @@
 #include "xfs_dquot_item.h"
 #include "xfs_dquot.h"
 #include "xfs_reflink.h"
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
+#include "xfs_dir2_priv.h"
+#include "xfs_attr_leaf.h"
+#include "xfs_shared.h"
 
 #include <linux/kthread.h>
 #include <linux/freezer.h>

@@ -449,6 +454,43 @@ xfs_iget_cache_hit(
 	return error;
 }
 
+/* Check inline fork contents. */
+int
+xfs_iget_verify_forks(
+	struct xfs_inode		*ip)
+{
+	int				error;
+
+	/* Check the attribute fork if there is one. */
+	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
+	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+		error = xfs_attr_shortform_verify(ip);
+		if (error) {
+			xfs_alert(ip->i_mount,
+					"%s: bad inode %Lu inline attr fork",
+					__func__, ip->i_ino);
+			return error;
+		}
+	}
+
+	/* Non-local data fork, jump out. */
+	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
+		return 0;
+
+	/* Check the inline data fork if there is one. */
+	if (S_ISDIR(VFS_I(ip)->i_mode))
+		error = xfs_dir2_sf_verify(ip);
+	else if (S_ISLNK(VFS_I(ip)->i_mode))
+		error = xfs_symlink_sf_verify(ip);
+	else
+		error = -EFSCORRUPTED;
+
+	if (error)
+		xfs_alert(ip->i_mount, "%s: bad inode %Lu inline data fork",
+				__func__, ip->i_ino);
+	return error;
+}
+
 
 static int
 xfs_iget_cache_miss(

@@ -473,6 +515,10 @@ xfs_iget_cache_miss(
 	if (error)
 		goto out_destroy;
 
+	error = xfs_iget_verify_forks(ip);
+	if (error)
+		goto out_destroy;
+
 	trace_xfs_iget_miss(ip);
 
 	if ((VFS_I(ip)->i_mode == 0) && !(flags & XFS_IGET_CREATE)) {

fs/xfs/xfs_icache.h +1 -0

index bff4d85..abf1cff 100644
@@ -129,5 +129,6 @@ xfs_fs_eofblocks_from_user(
 
 int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
 				  xfs_ino_t ino, bool *inuse);
+int xfs_iget_verify_forks(struct xfs_inode *ip);
 
 #endif

fs/xfs/xfs_inode.c +2 -4

index ceef77c..7ca8336 100644
@@ -3547,10 +3547,8 @@ xfs_iflush_int(
 	if (ip->i_d.di_version < 3)
 		ip->i_d.di_flushiter++;
 
-	/* Check the inline directory data. */
-	if (S_ISDIR(VFS_I(ip)->i_mode) &&
-	    ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
-	    xfs_dir2_sf_verify(ip))
+	/* Check the inline fork data. */
+	if (xfs_iget_verify_forks(ip))
 		goto corrupt_out;
 

Quoted 9 lines

Quoted 5 lines

fs/xfs/xfs_inode.c +0 -1

 	/*
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html