| Message ID | 20230719-ctime-v2-1-869825696d6d@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ext4: fix the time handling macros when ext4 is using small inodes | expand |
On Wed, Jul 19, 2023 at 06:32:19AM -0400, Jeff Layton wrote: > If ext4 is using small on-disk inodes, then it may not be able to store > fine grained timestamps. It also can't store the i_crtime at all in that > case since that fully lives in the extended part of the inode. > > 979492850abd got the EXT4_EINODE_{GET,SET}_XTIME macros wrong, and would > still store the tv_sec field of the i_crtime into the raw_inode, even > when they were small, corrupting adjacent memory. > > This fixes those macros to skip setting anything in the raw_inode if the > tv_sec field doesn't fit, and to properly return a {0,0} timestamp when > the raw_inode doesn't support it. > > Also, fix a bug in ctime handling during rename. It was updating the > renamed inode's ctime twice rather than the old directory. > > Cc: Jan Kara <jack@suse.cz> > Fixes: 979492850abd ("ext4: convert to ctime accessor functions") > Reported-by: Hugh Dickins <hughd@google.com> > Tested-by: Hugh Dickins <hughd@google.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> Acked-by: Theodore Ts'o <tytso@mit.edu> I assume this is will be applied to the vfs.ctime branch, yes? - Ted
On Thu, 2023-07-20 at 10:48 -0400, Theodore Ts'o wrote: > On Wed, Jul 19, 2023 at 06:32:19AM -0400, Jeff Layton wrote: > > If ext4 is using small on-disk inodes, then it may not be able to store > > fine grained timestamps. It also can't store the i_crtime at all in that > > case since that fully lives in the extended part of the inode. > > > > 979492850abd got the EXT4_EINODE_{GET,SET}_XTIME macros wrong, and would > > still store the tv_sec field of the i_crtime into the raw_inode, even > > when they were small, corrupting adjacent memory. > > > > This fixes those macros to skip setting anything in the raw_inode if the > > tv_sec field doesn't fit, and to properly return a {0,0} timestamp when > > the raw_inode doesn't support it. > > > > Also, fix a bug in ctime handling during rename. It was updating the > > renamed inode's ctime twice rather than the old directory. > > > > Cc: Jan Kara <jack@suse.cz> > > Fixes: 979492850abd ("ext4: convert to ctime accessor functions") > > Reported-by: Hugh Dickins <hughd@google.com> > > Tested-by: Hugh Dickins <hughd@google.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > Acked-by: Theodore Ts'o <tytso@mit.edu> > > I assume this is will be applied to the vfs.ctime branch, yes? > > - Ted Yes. Ideally it'll be folded into the ext4 patch there.
On Wed, 19 Jul 2023 06:32:19 -0400, Jeff Layton wrote: > If ext4 is using small on-disk inodes, then it may not be able to store > fine grained timestamps. It also can't store the i_crtime at all in that > case since that fully lives in the extended part of the inode. > > 979492850abd got the EXT4_EINODE_{GET,SET}_XTIME macros wrong, and would > still store the tv_sec field of the i_crtime into the raw_inode, even > when they were small, corrupting adjacent memory. > > [...] Applied to the vfs.ctime branch of the vfs/vfs.git tree. Patches in the vfs.ctime branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.ctime [1/1 FOLD] ext4: fix the time handling macros when ext4 is using small inodes https://git.kernel.org/vfs/vfs/c/1311011c2bb7
On Thu, Jul 20, 2023 at 10:54:16AM -0400, Jeff Layton wrote: > On Thu, 2023-07-20 at 10:48 -0400, Theodore Ts'o wrote: > > On Wed, Jul 19, 2023 at 06:32:19AM -0400, Jeff Layton wrote: > > > If ext4 is using small on-disk inodes, then it may not be able to store > > > fine grained timestamps. It also can't store the i_crtime at all in that > > > case since that fully lives in the extended part of the inode. > > > > > > 979492850abd got the EXT4_EINODE_{GET,SET}_XTIME macros wrong, and would > > > still store the tv_sec field of the i_crtime into the raw_inode, even > > > when they were small, corrupting adjacent memory. > > > > > > This fixes those macros to skip setting anything in the raw_inode if the > > > tv_sec field doesn't fit, and to properly return a {0,0} timestamp when > > > the raw_inode doesn't support it. > > > > > > Also, fix a bug in ctime handling during rename. It was updating the > > > renamed inode's ctime twice rather than the old directory. > > > > > > Cc: Jan Kara <jack@suse.cz> > > > Fixes: 979492850abd ("ext4: convert to ctime accessor functions") > > > Reported-by: Hugh Dickins <hughd@google.com> > > > Tested-by: Hugh Dickins <hughd@google.com> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > Acked-by: Theodore Ts'o <tytso@mit.edu> > > > > I assume this is will be applied to the vfs.ctime branch, yes? > > > > - Ted > > Yes. Ideally it'll be folded into the ext4 patch there. Done now, thanks!
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 2af347669db7..1e2259d9967d 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -900,8 +900,10 @@ do { \ #define EXT4_INODE_SET_CTIME(inode, raw_inode) \ EXT4_INODE_SET_XTIME_VAL(i_ctime, inode, raw_inode, inode_get_ctime(inode)) -#define EXT4_EINODE_SET_XTIME(xtime, einode, raw_inode) \ - EXT4_INODE_SET_XTIME_VAL(xtime, &((einode)->vfs_inode), raw_inode, (einode)->xtime) +#define EXT4_EINODE_SET_XTIME(xtime, einode, raw_inode) \ + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ + EXT4_INODE_SET_XTIME_VAL(xtime, &((einode)->vfs_inode), \ + raw_inode, (einode)->xtime) #define EXT4_INODE_GET_XTIME_VAL(xtime, inode, raw_inode) \ (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra) ? \ @@ -922,9 +924,14 @@ do { \ EXT4_INODE_GET_XTIME_VAL(i_ctime, inode, raw_inode)); \ } while (0) -#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) \ -do { \ - (einode)->xtime = EXT4_INODE_GET_XTIME_VAL(xtime, &(einode->vfs_inode), raw_inode); \ +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) \ +do { \ + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ + (einode)->xtime = \ + EXT4_INODE_GET_XTIME_VAL(xtime, &(einode->vfs_inode), \ + raw_inode); \ + else \ + (einode)->xtime = (struct timespec64){0, 0}; \ } while (0) #define i_disk_version osd1.linux1.l_i_version diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 07f6d96ebc60..933ad03f4f58 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -3957,7 +3957,7 @@ static int ext4_rename(struct mnt_idmap *idmap, struct inode *old_dir, ext4_dec_count(new.inode); inode_set_ctime_current(new.inode); } - old.dir->i_mtime = inode_set_ctime_current(old.inode); + old.dir->i_mtime = inode_set_ctime_current(old.dir); ext4_update_dx_flag(old.dir); if (old.dir_bh) { retval = ext4_rename_dir_finish(handle, &old, new.dir->i_ino);
If ext4 is using small on-disk inodes, then it may not be able to store fine grained timestamps. It also can't store the i_crtime at all in that case since that fully lives in the extended part of the inode. 979492850abd got the EXT4_EINODE_{GET,SET}_XTIME macros wrong, and would still store the tv_sec field of the i_crtime into the raw_inode, even when they were small, corrupting adjacent memory. This fixes those macros to skip setting anything in the raw_inode if the tv_sec field doesn't fit, and to properly return a {0,0} timestamp when the raw_inode doesn't support it. Also, fix a bug in ctime handling during rename. It was updating the renamed inode's ctime twice rather than the old directory. Cc: Jan Kara <jack@suse.cz> Fixes: 979492850abd ("ext4: convert to ctime accessor functions") Reported-by: Hugh Dickins <hughd@google.com> Tested-by: Hugh Dickins <hughd@google.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- Changes in v2: - also fix incorrect ctime update in ext4_rename --- fs/ext4/ext4.h | 17 ++++++++++++----- fs/ext4/namei.c | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) --- base-commit: c62e19541f8bb39f1f340247f651afe4532243df change-id: 20230718-ctime-f140dae8789d Best regards,