Set file mode during zfs_write #11576

aerusso · 2021-02-07T03:47:18Z

Motivation and Context

3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update.

3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function).

The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode.

Description

This patch restores that initialization.

How Has This Been Tested?

On a VM, it addresses the reproducer in #11474.

We may want to generate a reproducer for the ZTS.
I need a sanity check on this, @mattmacy @freqlabs @behlendorf .

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes openzfs#11474

ghost

Oof. Nice work finding that one.

ghost · 2021-02-08T16:17:33Z

If you can come up with a test case we can incorporate in ZTS great, but I'd like to see this merged either way.

behlendorf

The fix here looks good, thanks for running this one down. I agree it'd be nice to have a ZTS test for this, but I don't think it needs to hold up merging this since the fix is straightforward. I think the only somewhat subtle thing is that it is import that ip->i_mode also be updated but that will be handled in zfs_inode_update() a little latter in the function.

3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes #11474 Closes #11576

Apply openzfs/zfs#11576 Direct commit from upstream openzfs. Full commit message below: Set file mode during zfs_write 3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes #11474 Closes #11576 Obtained from: openzfs/zfs@f8ce8aed0 MFC after: 0 days Sponsored by: iXsystems, Inc.

Apply openzfs/zfs#11576 Direct commit from upstream openzfs. Full commit message below: Set file mode during zfs_write 3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes #11474 Closes #11576 Obtained from: openzfs/zfs@f8ce8aed0 MFC after: 0 days Sponsored by: iXsystems, Inc. (cherry picked from commit e9d419a)

See openzfs/zfs#11576 for details. Reviewed by: wg Approved by: wg (ports) Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D28554 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@564796 35697150-7ecd-e111-bb59-0022644237b5

See openzfs/zfs#11576 for details. Reviewed by: wg Approved by: wg (ports) Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D28554

sysutils/openzfs*: Bump to fix mode issue in zfs_write See openzfs/zfs#11576 for details. Approved by: portmgr (blanket, security/stability) Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D28554

Apply openzfs/zfs#11576 Direct commit from upstream openzfs. Full commit message below: Set file mode during zfs_write 3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes #11474 Closes #11576 Approved by: re (gdb) Obtained from: openzfs/zfs@f8ce8aed0 Sponsored by: iXsystems, Inc. (cherry picked from commit e9d419a) (cherry picked from commit 618dee6)

See openzfs/zfs#11576 for details. Reviewed by: wg Approved by: wg (ports) Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D28554 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@564796 35697150-7ecd-e111-bb59-0022644237b5

mjguzik · 2021-02-09T20:26:06Z

If I read this right the problem may have resulted in files with garbage mode hitting storage. Does 'scrub' or whatever other method scan and fix them? I presume some files might have received accidentally correct but unintended mode and that may be impossible to detect, but the rest should be fixable?

See openzfs/zfs#11576 for details. Reviewed by: wg Approved by: wg (ports) Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D28554

3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes openzfs#11474 Closes openzfs#11576

sysutils/openzfs*: Bump to fix mode issue in zfs_write See openzfs/zfs#11576 for details. Approved by: portmgr (blanket, security/stability) Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D28554

3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes openzfs#11474 Closes openzfs#11576

Apply openzfs/zfs#11576 Direct commit from upstream openzfs. Full commit message below: Set file mode during zfs_write 3d40b65 refactored zfs_vnops.c, which shared much code verbatim between Linux and BSD. After a successful write, the suid/sgid bits are reset, and the mode to be written is stored in newmode. On Linux, this was propagated to both the in-memory inode and znode, which is then updated with sa_update. 3d40b65 accidentally removed the initialization of newmode, which happened to occur on the same line as the inode update (which has been moved out of the function). The uninitialized newmode can be saved to disk, leading to a crash on stat() of that file, in addition to a merely incorrect file mode. Reviewed-by: Ryan Moeller <ryan@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Antonio Russo <aerusso@aerusso.net> Closes #11474 Closes #11576 Obtained from: openzfs/zfs@f8ce8aed0 MFC after: 0 days Sponsored by: iXsystems, Inc.

ghost approved these changes Feb 8, 2021

View reviewed changes

ghost added the Status: Code Review Needed Ready for review and testing label Feb 8, 2021

behlendorf approved these changes Feb 8, 2021

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 8, 2021

behlendorf merged commit f8ce8ae into openzfs:master Feb 8, 2021

wizeman mentioned this pull request Feb 14, 2021

zfs: 2.0.2 -> 2.0.3 NixOS/nixpkgs#112910

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set file mode during zfs_write #11576

Set file mode during zfs_write #11576

aerusso commented Feb 7, 2021

ghost left a comment

ghost commented Feb 8, 2021

behlendorf left a comment

mjguzik commented Feb 9, 2021 •

edited

Loading

Set file mode during zfs_write #11576

Set file mode during zfs_write #11576

Conversation

aerusso commented Feb 7, 2021

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ghost left a comment

Choose a reason for hiding this comment

ghost commented Feb 8, 2021

behlendorf left a comment

Choose a reason for hiding this comment

mjguzik commented Feb 9, 2021 • edited Loading

mjguzik commented Feb 9, 2021 •

edited

Loading