linux-kernel - Re: [PATCH RFC] nfsd: set missing after_change as before

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Jul 2023 16:44:59 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>
CC:     Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <dai.ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] nfsd: set missing after_change as before_change + 1



> On Jul 24, 2023, at 12:21 PM, Jeff Layton <jlayton@...nel.org> wrote:
> 
> On Mon, 2023-07-24 at 11:20 -0400, Chuck Lever wrote:
>> On Mon, Jul 24, 2023 at 10:53:39AM -0400, Jeff Layton wrote:
>>> In the event that we can't fetch post_op_attr attributes, we still need
>>> to set a value for the after_change. The operation has already happened,
>>> so we're not able to return an error at that point, but we do want to
>>> ensure that the client knows that its cache should be invalidated.
>>> 
>>> If we weren't able to fetch post-op attrs, then just set the
>>> after_change to before_change + 1. The atomic flag should already be
>>> clear in this case.
>>> 
>>> Suggested-by: Neil Brown <neilb@...e.de>
>>> Signed-off-by: Jeff Layton <jlayton@...nel.org>
>>> ---
>>> fs/nfsd/nfs4proc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> I'm not sure this change makes any difference. The client would
>> possibly see the change value move forward then back. I'd think a
>> false "atomic" field and using the /same/ pre- and post-change would
>> be safer...?
>> 
>> But I'm intrigued enough to apply this to nfsd-next provisionally,
>> at least for testing and further review. It will appear a little
>> later today.
>> 
>> 
> 
> Thanks. I think there really are no great choices here.
> 
> This is a rather unlikely error case that should only come into play
> when there are problems with the underlying filesystem, but only when
> fetching the post-op attrs.
> 
> We don't have a way to just opt out of providing a post-op attribute and
> I think this is probably the least bad option of what to put in there.

No argument, it's a rock-and-hard-place thing.

There doesn't seem to be a way of testing this except
with fault injection.

Any client implementer that has an opinion about our
choice of post-change value (zero versus pre-change
versus pre-change-plus-one), please chime in.


>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 3f6710c9c5c9..f0f318e78630 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -411,7 +411,7 @@ set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
>>> if (WARN_ON_ONCE(!fhp->fh_pre_saved))
>>> cinfo->before_change = 0;
>>> if (!fhp->fh_post_saved)
>>> - cinfo->after_change = 0;
>>> + cinfo->after_change = cinfo->before_change + 1;
>>> }
>>> 
>>> static __be32
>>> 
>>> ---
>>> base-commit: 97a5d0146ef443df148805a4e9c3c44111f14ab1
>>> change-id: 20230724-bz2223560-5ed6bc3a5db7
>>> 
>>> Best regards,
>>> -- 
>>> Jeff Layton <jlayton@...nel.org>
>>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@...nel.org>


--
Chuck Lever