| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <67d99140-0513-4797-92f8-1375a06f689a@oracle.com>
Date: Sat, 26 Jul 2025 15:57:59 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>,
Alexander Viro
<viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
NeilBrown <neil@...wn.name>, Olga Kornievskaia <okorniev@...hat.com>,
Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>
Cc: Trond Myklebust <trondmy@...merspace.com>,
Anna Schumaker <anna@...nel.org>, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
linux-nfs@...r.kernel.org
Subject: Re: [PATCH v2 3/7] nfsd: use ATTR_CTIME_SET for delegated ctime
updates
On 7/26/25 3:03 PM, Jeff Layton wrote:
> On Sat, 2025-07-26 at 14:48 -0400, Chuck Lever wrote:
>> Hi Jeff -
>>
>> Thanks again for your focus on getting this straightened out!
>>
>>
>> On 7/26/25 10:31 AM, Jeff Layton wrote:
>>> Ensure that notify_change() doesn't clobber a delegated ctime update
>>> with current_time() by setting ATTR_CTIME_SET for those updates.
>>>
>>> Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct
>>> value.
>>
>> I don't yet see the connection of the above tv_nsec fix to the other
>> changes in this patch. Wouldn't this be an independent fix?
>>
>
> I felt like they were related. Yes, the ia_ctime field is currently
> being set wrong, but it's also being clobbered by notify_change(), so
> it doesn't matter much. I can break this into a separate patch (with a
> Fixes: tag) if you prefer though.
Ah, got it, this patch exposes a latent bug. The usual thing to do is to
fix the latent bug in a preceding/pre-requisite patch, so that's my
preference.
>>> Don't bother setting the timestamps in cb_getattr_update_times() in the
>>> non-delegated case. notify_change() will do that itself.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@...nel.org>
>>
>> General comments:
>>
>> I don't feel that any of the patches in this series need to be tagged
>> for stable, since there is already a Kconfig setting that defaults to
>> leaving timestamp delegation disabled. But I would like to see Fixes:
>> tags, where that makes sense?
>>
>
> I don't think any of these need to go to stable since this is still
> under a non-default Kconfig option, and the main effect of the bug is
> wonky timestamps. I should be able to add some Fixes: tags though.
>
>> Is this set on top of the set you posted a day or two ago with the new
>> trace point? Or does this set replace that one?
>>
>
> This set should replace those.
I was confused because the trace point patch is missing, and dropping it
wasn't mentioned in the cover letter's Change log. NBD, thanks for
clarifying.
Since the bulk of these are NFSD changes, I volunteer to take v3 once
we have Acks from the VFS maintainers, as needed.
>>> ---
>>> fs/nfsd/nfs4state.c | 6 +++---
>>> fs/nfsd/nfs4xdr.c | 5 +++--
>>> 2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
>>> static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
>>> {
>>> struct inode *inode = d_inode(dentry);
>>> - struct timespec64 now = current_time(inode);
>>> struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
>>> struct iattr attrs = { };
>>> int ret;
>>> @@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>>> if (deleg_attrs_deleg(dp->dl_type)) {
>>> struct timespec64 atime = inode_get_atime(inode);
>>> struct timespec64 mtime = inode_get_mtime(inode);
>>> + struct timespec64 now = current_time(inode);
>>>
>>> attrs.ia_atime = ncf->ncf_cb_atime;
>>> attrs.ia_mtime = ncf->ncf_cb_mtime;
>>> @@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>>> attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
>>>
>>> if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
>>> - attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
>>> + attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
>>> + ATTR_MTIME | ATTR_MTIME_SET;
>>> attrs.ia_ctime = attrs.ia_mtime;
>>> }
>>> } else {
>>> attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
>>> - attrs.ia_mtime = attrs.ia_ctime = now;
>>> }
>>>
>>> if (!attrs.ia_valid)
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
>>> iattr->ia_mtime.tv_sec = modify.seconds;
>>> iattr->ia_mtime.tv_nsec = modify.nseconds;
>>> iattr->ia_ctime.tv_sec = modify.seconds;
>>> - iattr->ia_ctime.tv_nsec = modify.seconds;
>>> - iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
>>> + iattr->ia_ctime.tv_nsec = modify.nseconds;
>>> + iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
>>> + ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
>>> }
>>>
>>> /* request sanity: did attrlist4 contain the expected number of words? */
>>>
>>
>>
>
--
Chuck Lever
Powered by blists - more mailing lists