Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <808140.1685573412@warthog.procyon.org.uk>
In-Reply-To: <808140.1685573412@warthog.procyon.org.uk>
From:   Marc Dionne <marc.dionne@auristor.com>
Date:   Thu, 1 Jun 2023 10:25:42 -0300
Message-ID: <CAB9dFduJtzreY9dY0oa0oNtXBOWzRyCDZ45zPQR4neJBBuj94Q@mail.gmail.com>
Subject: Re: [PATCH] afs: Fix setting of mtime when creating a file/dir/symlink
To:     David Howells <dhowells@redhat.com>
Cc:     linux-afs@lists.infradead.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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