[PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

[PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

[David Howells]

kafs incorrectly passes a zero mtime (ie. 1st Jan 1970) to the server when
creating a file, dir or symlink because the mtime recorded in the
afs_operation struct gets passed to the server by the marshalling routines,
but the afs_mkdir(), afs_create() and afs_symlink() functions don't set it.

This gets masked if a file or directory is subsequently modified.

Fix this by filling in op->mtime before calling the create op.

Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept")
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: linux-fsdevel@vger.kernel.org
---
 fs/afs/dir.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 4dd97afa536c..5219182e52e1 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -1358,6 +1358,7 @@ static int afs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	op->dentry	= dentry;
 	op->create.mode	= S_IFDIR | mode;
 	op->create.reason = afs_edit_dir_for_mkdir;
+	op->mtime	= current_time(dir);
 	op->ops		= &afs_mkdir_operation;
 	return afs_do_sync_operation(op);
 }
@@ -1661,6 +1662,7 @@ static int afs_create(struct mnt_idmap *idmap, struct inode *dir,
 	op->dentry	= dentry;
 	op->create.mode	= S_IFREG | mode;
 	op->create.reason = afs_edit_dir_for_create;
+	op->mtime	= current_time(dir);
 	op->ops		= &afs_create_operation;
 	return afs_do_sync_operation(op);
 
@@ -1796,6 +1798,7 @@ static int afs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	op->ops			= &afs_symlink_operation;
 	op->create.reason	= afs_edit_dir_for_symlink;
 	op->create.symlink	= content;
+	op->mtime		= current_time(dir);
 	return afs_do_sync_operation(op);
 
 error:

Re: [PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

[David Howells]

Hi Linus,

Sorry, I forgot to say in the patch email, but could you apply this please?

Thanks,
David

[PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

[David Howells]

    
kafs incorrectly passes a zero mtime (ie. 1st Jan 1970) to the server when
creating a file, dir or symlink because commit 52af7105eceb caused the
mtime recorded in the afs_operation struct to be passed to the server, but
didn't modify the afs_mkdir(), afs_create() and afs_symlink() functions to
set it first.

Those functions were written with the assumption that the mtime would be
obtained from the server - but that fell foul of malsynchronised clocks, so
it was decided that the mtime should be set from the client instead.

Fix this by filling in op->mtime before calling the create op.

Fixes: 52af7105eceb ("afs: Set mtime from the client for yfs create operations")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: linux-fsdevel@vger.kernel.org
---
 fs/afs/dir.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 4dd97afa536c..5219182e52e1 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -1358,6 +1358,7 @@ static int afs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	op->dentry	= dentry;
 	op->create.mode	= S_IFDIR | mode;
 	op->create.reason = afs_edit_dir_for_mkdir;
+	op->mtime	= current_time(dir);
 	op->ops		= &afs_mkdir_operation;
 	return afs_do_sync_operation(op);
 }
@@ -1661,6 +1662,7 @@ static int afs_create(struct mnt_idmap *idmap, struct inode *dir,
 	op->dentry	= dentry;
 	op->create.mode	= S_IFREG | mode;
 	op->create.reason = afs_edit_dir_for_create;
+	op->mtime	= current_time(dir);
 	op->ops		= &afs_create_operation;
 	return afs_do_sync_operation(op);
 
@@ -1796,6 +1798,7 @@ static int afs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	op->ops			= &afs_symlink_operation;
 	op->create.reason	= afs_edit_dir_for_symlink;
 	op->create.symlink	= content;
+	op->mtime		= current_time(dir);
 	return afs_do_sync_operation(op);
 
 error:

Re: [PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On 5/31/2023 6:50 PM, David Howells wrote:
>      
> kafs incorrectly passes a zero mtime (ie. 1st Jan 1970) to the server when
> creating a file, dir or symlink because commit 52af7105eceb caused the
> mtime recorded in the afs_operation struct to be passed to the server, but
> didn't modify the afs_mkdir(), afs_create() and afs_symlink() functions to
> set it first.
>
> Those functions were written with the assumption that the mtime would be
> obtained from the server - but that fell foul of malsynchronised clocks, so
> it was decided that the mtime should be set from the client instead.
>
> Fix this by filling in op->mtime before calling the create op.
>
> Fixes: 52af7105eceb ("afs: Set mtime from the client for yfs create operations")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: linux-fsdevel@vger.kernel.org
> ---
>   fs/afs/dir.c |    3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 4dd97afa536c..5219182e52e1 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -1358,6 +1358,7 @@ static int afs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>   	op->dentry	= dentry;
>   	op->create.mode	= S_IFDIR | mode;
>   	op->create.reason = afs_edit_dir_for_mkdir;
> +	op->mtime	= current_time(dir);
>   	op->ops		= &afs_mkdir_operation;
>   	return afs_do_sync_operation(op);
>   }
> @@ -1661,6 +1662,7 @@ static int afs_create(struct mnt_idmap *idmap, struct inode *dir,
>   	op->dentry	= dentry;
>   	op->create.mode	= S_IFREG | mode;
>   	op->create.reason = afs_edit_dir_for_create;
> +	op->mtime	= current_time(dir);
>   	op->ops		= &afs_create_operation;
>   	return afs_do_sync_operation(op);
>   
> @@ -1796,6 +1798,7 @@ static int afs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>   	op->ops			= &afs_symlink_operation;
>   	op->create.reason	= afs_edit_dir_for_symlink;
>   	op->create.symlink	= content;
> +	op->mtime		= current_time(dir);
>   	return afs_do_sync_operation(op);
>   
>   error:
>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]

Re: [PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

[Marc Dionne]

On Wed, May 31, 2023 at 7:50 PM David Howells <dhowells@redhat.com> wrote:
>
>
> kafs incorrectly passes a zero mtime (ie. 1st Jan 1970) to the server when
> creating a file, dir or symlink because commit 52af7105eceb caused the
> mtime recorded in the afs_operation struct to be passed to the server, but
> didn't modify the afs_mkdir(), afs_create() and afs_symlink() functions to
> set it first.
>
> Those functions were written with the assumption that the mtime would be
> obtained from the server - but that fell foul of malsynchronised clocks, so
> it was decided that the mtime should be set from the client instead.
>
> Fix this by filling in op->mtime before calling the create op.
>
> Fixes: 52af7105eceb ("afs: Set mtime from the client for yfs create operations")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/afs/dir.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 4dd97afa536c..5219182e52e1 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -1358,6 +1358,7 @@ static int afs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>         op->dentry      = dentry;
>         op->create.mode = S_IFDIR | mode;
>         op->create.reason = afs_edit_dir_for_mkdir;
> +       op->mtime       = current_time(dir);
>         op->ops         = &afs_mkdir_operation;
>         return afs_do_sync_operation(op);
>  }
> @@ -1661,6 +1662,7 @@ static int afs_create(struct mnt_idmap *idmap, struct inode *dir,
>         op->dentry      = dentry;
>         op->create.mode = S_IFREG | mode;
>         op->create.reason = afs_edit_dir_for_create;
> +       op->mtime       = current_time(dir);
>         op->ops         = &afs_create_operation;
>         return afs_do_sync_operation(op);
>
> @@ -1796,6 +1798,7 @@ static int afs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>         op->ops                 = &afs_symlink_operation;
>         op->create.reason       = afs_edit_dir_for_symlink;
>         op->create.symlink      = content;
> +       op->mtime               = current_time(dir);
>         return afs_do_sync_operation(op);
>
>  error:

The fix looks good, but as we discussed privately, the issue that this
fixes predates commit 52af7105eceb.  That commit only touched the yfs
client code and made it rely on the op mtime rather than letting the
server set the time. This made it inherit the issue that was already
present for the non yfs client code.

Marc