[PATCH v2] ext4: fix the time handling macros when ext4 is using small inodes

[PATCH v2] ext4: fix the time handling macros when ext4 is using small inodes

[Jeff Layton]

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(-)

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);

---
base-commit: c62e19541f8bb39f1f340247f651afe4532243df
change-id: 20230718-ctime-f140dae8789d

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] ext4: fix the time handling macros when ext4 is using small inodes

[Theodore Ts'o]

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

Re: [PATCH v2] ext4: fix the time handling macros when ext4 is using small inodes

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] ext4: fix the time handling macros when ext4 is using small inodes

[Christian Brauner]

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

Re: [PATCH v2] ext4: fix the time handling macros when ext4 is using small inodes

[Christian Brauner]

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!