From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-05-25 06:15:33
From: Darrick J. Wong <djwong@kernel.org>
The RTINHERIT bit can be set on a directory so that newly created
regular files will have the REALTIME bit set to store their data on the
realtime volume. If an extent size hint (and EXTSZINHERIT) are set on
the directory, the hint will also be copied into the new file.
As pointed out in previous patches, for realtime files we require the
extent size hint be an integer multiple of the realtime extent, but we
don't perform the same validation on a directory with both RTINHERIT and
EXTSZINHERIT set, even though the only use-case of that combination is
to propagate extent size hints into new realtime files. This leads to
inode corruption errors when the bad values are propagated.
Because there may be existing filesystems with such a configuration, we
cannot simply amend the inode verifier to trip on these directories and
call it a day because that will cause previously "working" filesystems
to start throwing errors abruptly. Note that it's valid to have
directories with rtinherit set even if there is no realtime volume, in
which case the problem does not manifest because rtinherit is ignored if
there's no realtime device; and it's possible that someone set the flag,
crashed, repaired the filesystem (which clears the hint on the realtime
file) and continued.
Therefore, mitigate this issue in several ways: First, if we try to
write out an inode with both rtinherit/extszinherit set and an unaligned
extent size hint, turn off the hint to correct the error. Second, if
someone tries to misconfigure a directory via the fssetxattr ioctl, fail
the ioctl. Third, reverify both extent size hint values when we
propagate heritable inode attributes from parent to child, to prevent
misconfigurations from spreading.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: disable incorrect hints at runtime instead of whacking filesystems
with verifier errors
v3: revise the comment in the verifier to describe the source of the
problem, the observable symptoms, and how the solution fits the
historical context
---
fs/xfs/libxfs/xfs_inode_buf.c | 22 ++++++++++++++++++++++
fs/xfs/libxfs/xfs_trans_inode.c | 15 +++++++++++++++
fs/xfs/xfs_inode.c | 29 +++++++++++++++++++++++++++++
fs/xfs/xfs_ioctl.c | 15 +++++++++++++++
4 files changed, 81 insertions(+)
@@ -729,6 +730,24 @@ xfs_inode_inherit_flags(di_flags|=XFS_DIFLAG_FILESTREAM;ip->i_diflags|=di_flags;++/*+*Inodeverifiersonolderkernelsonlycheckthattheextentsize+*hintisanintegermultipleofthertextentsizeonrealtimefiles.+*Theydidnotcheckthehintalignmentonadirectorywithboth+*rtinheritandextszinheritflagsset.Ifthemisalignedhintis+*propagatedfromadirectoryintoanewrealtimefile,newfile+*allocationswillfailduetomatherrorsinthertallocatorand/or+*triptheverifiers.Validatethehintsettingsinthenewfileso+*thatwedon'tletbrokenhintspropagate.+*/+failaddr=xfs_inode_validate_extsize(ip->i_mount,ip->i_extsize,+VFS_I(ip)->i_mode,ip->i_diflags);+if(failaddr){+ip->i_diflags&=~(XFS_DIFLAG_EXTSIZE|+XFS_DIFLAG_EXTSZINHERIT);+ip->i_extsize=0;+}}/* Propagate di_flags2 from a parent inode to a child inode. */
From: Carlos Maiolino <hidden> Date: 2021-05-25 10:49:12
Hi Darrick.
On Mon, May 24, 2021 at 11:15:31PM -0700, Darrick J. Wong wrote:
From: Darrick J. Wong <djwong@kernel.org>
The RTINHERIT bit can be set on a directory so that newly created
regular files will have the REALTIME bit set to store their data on the
realtime volume. If an extent size hint (and EXTSZINHERIT) are set on
the directory, the hint will also be copied into the new file.
As pointed out in previous patches, for realtime files we require the
extent size hint be an integer multiple of the realtime extent, but we
don't perform the same validation on a directory with both RTINHERIT and
EXTSZINHERIT set, even though the only use-case of that combination is
to propagate extent size hints into new realtime files. This leads to
inode corruption errors when the bad values are propagated.
Because there may be existing filesystems with such a configuration, we
cannot simply amend the inode verifier to trip on these directories and
call it a day because that will cause previously "working" filesystems
to start throwing errors abruptly. Note that it's valid to have
directories with rtinherit set even if there is no realtime volume, in
which case the problem does not manifest because rtinherit is ignored if
there's no realtime device; and it's possible that someone set the flag,
crashed, repaired the filesystem (which clears the hint on the realtime
file) and continued.
Therefore, mitigate this issue in several ways: First, if we try to
write out an inode with both rtinherit/extszinherit set and an unaligned
extent size hint, turn off the hint to correct the error. Second, if
someone tries to misconfigure a directory via the fssetxattr ioctl, fail
the ioctl. Third, reverify both extent size hint values when we
propagate heritable inode attributes from parent to child, to prevent
misconfigurations from spreading.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: disable incorrect hints at runtime instead of whacking filesystems
with verifier errors
v3: revise the comment in the verifier to describe the source of the
problem, the observable symptoms, and how the solution fits the
historical context
IMHO the patch is fine, I have just one comment I'd like to address though:
quoted hunk
+ /*+ * Inode verifiers on older kernels don't check that the extent size+ * hint is an integer multiple of the rt extent size on a directory+ * with both rtinherit and extszinherit flags set. If we're logging a+ * directory that is misconfigured in this way, clear the hint.+ */+ if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&+ (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&+ (ip->i_extsize % ip->i_mount->m_sb.sb_rextsize) > 0) {+ ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |+ XFS_DIFLAG_EXTSZINHERIT);+ ip->i_extsize = 0;+ flags |= XFS_ILOG_CORE;+ }+
...
quoted hunk
+ * that we don't let broken hints propagate.+ */+ failaddr = xfs_inode_validate_extsize(ip->i_mount, ip->i_extsize,+ VFS_I(ip)->i_mode, ip->i_diflags);+ if (failaddr) {+ ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |+ XFS_DIFLAG_EXTSZINHERIT);+ ip->i_extsize = 0;+ } }
In all cases above, wouldn't be interesting to at least log the fact we are
resetting the extent size? At least in debug mode? This may let users clueless
on why the extent size has been reset, or at least give us some debug data when
required?
The patch itself looks fine, with or without logging the extsize reset, you can
add:
Reviewed-by: Carlos Maiolino <redacted>
Cheers
From: Brian Foster <hidden> Date: 2021-05-25 11:54:11
On Mon, May 24, 2021 at 11:15:31PM -0700, Darrick J. Wong wrote:
From: Darrick J. Wong <djwong@kernel.org>
The RTINHERIT bit can be set on a directory so that newly created
regular files will have the REALTIME bit set to store their data on the
realtime volume. If an extent size hint (and EXTSZINHERIT) are set on
the directory, the hint will also be copied into the new file.
As pointed out in previous patches, for realtime files we require the
extent size hint be an integer multiple of the realtime extent, but we
don't perform the same validation on a directory with both RTINHERIT and
EXTSZINHERIT set, even though the only use-case of that combination is
to propagate extent size hints into new realtime files. This leads to
inode corruption errors when the bad values are propagated.
Because there may be existing filesystems with such a configuration, we
cannot simply amend the inode verifier to trip on these directories and
call it a day because that will cause previously "working" filesystems
to start throwing errors abruptly. Note that it's valid to have
directories with rtinherit set even if there is no realtime volume, in
which case the problem does not manifest because rtinherit is ignored if
there's no realtime device; and it's possible that someone set the flag,
crashed, repaired the filesystem (which clears the hint on the realtime
file) and continued.
Therefore, mitigate this issue in several ways: First, if we try to
write out an inode with both rtinherit/extszinherit set and an unaligned
extent size hint, turn off the hint to correct the error. Second, if
someone tries to misconfigure a directory via the fssetxattr ioctl, fail
the ioctl. Third, reverify both extent size hint values when we
propagate heritable inode attributes from parent to child, to prevent
misconfigurations from spreading.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: disable incorrect hints at runtime instead of whacking filesystems
with verifier errors
v3: revise the comment in the verifier to describe the source of the
problem, the observable symptoms, and how the solution fits the
historical context
---
I agree with Carlos in that I'd prefer to see some kind of one-shot
warning to indicate that the problem has been identified in the fs. Some
users may want to run a repair sequence (or would otherwise have no idea
they should consider one) just to put things back in a correct state.
(Perhaps putting the clearing code in a common helper with a flag to
toggle between extszhint and cowextszhint would facilitate?).
That aside, the patch looks Ok to me:
Reviewed-by: Brian Foster <redacted>
@@ -729,6 +730,24 @@ xfs_inode_inherit_flags(di_flags|=XFS_DIFLAG_FILESTREAM;ip->i_diflags|=di_flags;++/*+*Inodeverifiersonolderkernelsonlycheckthattheextentsize+*hintisanintegermultipleofthertextentsizeonrealtimefiles.+*Theydidnotcheckthehintalignmentonadirectorywithboth+*rtinheritandextszinheritflagsset.Ifthemisalignedhintis+*propagatedfromadirectoryintoanewrealtimefile,newfile+*allocationswillfailduetomatherrorsinthertallocatorand/or+*triptheverifiers.Validatethehintsettingsinthenewfileso+*thatwedon'tletbrokenhintspropagate.+*/+failaddr=xfs_inode_validate_extsize(ip->i_mount,ip->i_extsize,+VFS_I(ip)->i_mode,ip->i_diflags);+if(failaddr){+ip->i_diflags&=~(XFS_DIFLAG_EXTSIZE|+XFS_DIFLAG_EXTSZINHERIT);+ip->i_extsize=0;+}}/* Propagate di_flags2 from a parent inode to a child inode. */
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-05-25 17:21:03
On Tue, May 25, 2021 at 12:49:02PM +0200, Carlos Maiolino wrote:
Hi Darrick.
On Mon, May 24, 2021 at 11:15:31PM -0700, Darrick J. Wong wrote:
quoted
From: Darrick J. Wong <djwong@kernel.org>
The RTINHERIT bit can be set on a directory so that newly created
regular files will have the REALTIME bit set to store their data on the
realtime volume. If an extent size hint (and EXTSZINHERIT) are set on
the directory, the hint will also be copied into the new file.
As pointed out in previous patches, for realtime files we require the
extent size hint be an integer multiple of the realtime extent, but we
don't perform the same validation on a directory with both RTINHERIT and
EXTSZINHERIT set, even though the only use-case of that combination is
to propagate extent size hints into new realtime files. This leads to
inode corruption errors when the bad values are propagated.
Because there may be existing filesystems with such a configuration, we
cannot simply amend the inode verifier to trip on these directories and
call it a day because that will cause previously "working" filesystems
to start throwing errors abruptly. Note that it's valid to have
directories with rtinherit set even if there is no realtime volume, in
which case the problem does not manifest because rtinherit is ignored if
there's no realtime device; and it's possible that someone set the flag,
crashed, repaired the filesystem (which clears the hint on the realtime
file) and continued.
Therefore, mitigate this issue in several ways: First, if we try to
write out an inode with both rtinherit/extszinherit set and an unaligned
extent size hint, turn off the hint to correct the error. Second, if
someone tries to misconfigure a directory via the fssetxattr ioctl, fail
the ioctl. Third, reverify both extent size hint values when we
propagate heritable inode attributes from parent to child, to prevent
misconfigurations from spreading.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: disable incorrect hints at runtime instead of whacking filesystems
with verifier errors
v3: revise the comment in the verifier to describe the source of the
problem, the observable symptoms, and how the solution fits the
historical context
IMHO the patch is fine, I have just one comment I'd like to address though:
quoted
+ /*+ * Inode verifiers on older kernels don't check that the extent size+ * hint is an integer multiple of the rt extent size on a directory+ * with both rtinherit and extszinherit flags set. If we're logging a+ * directory that is misconfigured in this way, clear the hint.+ */+ if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&+ (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&+ (ip->i_extsize % ip->i_mount->m_sb.sb_rextsize) > 0) {+ ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |+ XFS_DIFLAG_EXTSZINHERIT);+ ip->i_extsize = 0;+ flags |= XFS_ILOG_CORE;+ }+
...
quoted
+ * that we don't let broken hints propagate.+ */+ failaddr = xfs_inode_validate_extsize(ip->i_mount, ip->i_extsize,+ VFS_I(ip)->i_mode, ip->i_diflags);+ if (failaddr) {+ ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |+ XFS_DIFLAG_EXTSZINHERIT);+ ip->i_extsize = 0;+ } }
In all cases above, wouldn't be interesting to at least log the fact we are
resetting the extent size? At least in debug mode? This may let users clueless
on why the extent size has been reset, or at least give us some debug data when
required?
Ok, I'll add an xfs_info message to log the fact that we are changing
inode attributes. Thanks for the review!
--D
The patch itself looks fine, with or without logging the extsize reset, you can
add:
Reviewed-by: Carlos Maiolino <redacted>
Cheers
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-05-25 17:29:38
On Tue, May 25, 2021 at 07:54:04AM -0400, Brian Foster wrote:
On Mon, May 24, 2021 at 11:15:31PM -0700, Darrick J. Wong wrote:
quoted
From: Darrick J. Wong <djwong@kernel.org>
The RTINHERIT bit can be set on a directory so that newly created
regular files will have the REALTIME bit set to store their data on the
realtime volume. If an extent size hint (and EXTSZINHERIT) are set on
the directory, the hint will also be copied into the new file.
As pointed out in previous patches, for realtime files we require the
extent size hint be an integer multiple of the realtime extent, but we
don't perform the same validation on a directory with both RTINHERIT and
EXTSZINHERIT set, even though the only use-case of that combination is
to propagate extent size hints into new realtime files. This leads to
inode corruption errors when the bad values are propagated.
Because there may be existing filesystems with such a configuration, we
cannot simply amend the inode verifier to trip on these directories and
call it a day because that will cause previously "working" filesystems
to start throwing errors abruptly. Note that it's valid to have
directories with rtinherit set even if there is no realtime volume, in
which case the problem does not manifest because rtinherit is ignored if
there's no realtime device; and it's possible that someone set the flag,
crashed, repaired the filesystem (which clears the hint on the realtime
file) and continued.
Therefore, mitigate this issue in several ways: First, if we try to
write out an inode with both rtinherit/extszinherit set and an unaligned
extent size hint, turn off the hint to correct the error. Second, if
someone tries to misconfigure a directory via the fssetxattr ioctl, fail
the ioctl. Third, reverify both extent size hint values when we
propagate heritable inode attributes from parent to child, to prevent
misconfigurations from spreading.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: disable incorrect hints at runtime instead of whacking filesystems
with verifier errors
v3: revise the comment in the verifier to describe the source of the
problem, the observable symptoms, and how the solution fits the
historical context
---
I agree with Carlos in that I'd prefer to see some kind of one-shot
warning to indicate that the problem has been identified in the fs. Some
users may want to run a repair sequence (or would otherwise have no idea
they should consider one) just to put things back in a correct state.
Done. xfs_trans_log_inode now has:
/*
* Inode verifiers on older kernels don't check that the extent size
* hint is an integer multiple of the rt extent size on a directory
* with both rtinherit and extszinherit flags set. If we're logging a
* directory that is misconfigured in this way, clear the hint.
*/
if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
(ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
(ip->i_extsize % ip->i_mount->m_sb.sb_rextsize) > 0) {
xfs_info_once(ip->i_mount,
"Correcting misaligned extent size hint in inode 0x%llx.", ip->i_ino);
ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
XFS_DIFLAG_EXTSZINHERIT);
ip->i_extsize = 0;
flags |= XFS_ILOG_CORE;
}
(Perhaps putting the clearing code in a common helper with a flag to
toggle between extszhint and cowextszhint would facilitate?).
The cowextsize hint shouldn't have this problem since you can't set the
cow hint unless reflink is enabled, and you can't enable reflink if
there's a realtime volume attached. The appropriate extra checks will
need to be added to xfs_inode_validate_cowextsize, of course.
That aside, the patch looks Ok to me:
Reviewed-by: Brian Foster <redacted>
@@ -729,6 +730,24 @@ xfs_inode_inherit_flags(di_flags|=XFS_DIFLAG_FILESTREAM;ip->i_diflags|=di_flags;++/*+*Inodeverifiersonolderkernelsonlycheckthattheextentsize+*hintisanintegermultipleofthertextentsizeonrealtimefiles.+*Theydidnotcheckthehintalignmentonadirectorywithboth+*rtinheritandextszinheritflagsset.Ifthemisalignedhintis+*propagatedfromadirectoryintoanewrealtimefile,newfile+*allocationswillfailduetomatherrorsinthertallocatorand/or+*triptheverifiers.Validatethehintsettingsinthenewfileso+*thatwedon'tletbrokenhintspropagate.+*/+failaddr=xfs_inode_validate_extsize(ip->i_mount,ip->i_extsize,+VFS_I(ip)->i_mode,ip->i_diflags);+if(failaddr){+ip->i_diflags&=~(XFS_DIFLAG_EXTSIZE|+XFS_DIFLAG_EXTSZINHERIT);+ip->i_extsize=0;+}}/* Propagate di_flags2 from a parent inode to a child inode. */