linux-kernel - Re: [GIT PULL] nfsd changes for 5.18

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]

Message-ID: <20220711183603.GD14184@fieldses.org>
Date:   Mon, 11 Jul 2022 14:36:03 -0400
From:   Bruce Fields <bfields@...ldses.org>
To:     Chuck Lever III <chuck.lever@...cle.com>
Cc:     Jeff Layton <jlayton@...hat.com>,
        Igor Mammedov <imammedo@...hat.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Ondrej Valousek <ondrej.valousek.xm@...esas.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [GIT PULL] nfsd changes for 5.18

On Mon, Jul 11, 2022 at 06:24:01PM +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 11, 2022, at 2:19 PM, Bruce Fields <bfields@...ldses.org> wrote:
> > 
> > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> >> On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> >>>> This patch regressed clients that support TIME_CREATE attribute.
> >>>> Starting with this patch client might think that server supports
> >>>> TIME_CREATE and start sending this attribute in its requests.
> >>> 
> >>> Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> >>> attribute") does not include a change to nfsd4_decode_fattr4()
> >>> that decodes the birth time attribute.
> >>> 
> >>> I don't immediately see another storage protocol stack in our
> >>> kernel that supports a client setting the birth time, so NFSD
> >>> might have to ignore the client-provided value.
> >>> 
> >> 
> >> Cephfs allows this. My thinking at the time that I implemented it was
> >> that it should be settable for backup purposes, but this was possibly a
> >> mistake. On most filesystems, the btime seems to be equivalent to inode
> >> creation time and is read-only.
> > 
> > So supporting it as read-only seems reasonable.
> > 
> > Clearly, failing to decode the setattr attempt isn't the right way to do
> > that.  I'm not sure what exactly it should be doing--some kind of
> > permission error on any setattr containing TIME_CREATE?
> 
> I don't think that will work.
> 
> NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
> the mask of attributes it supports. That means the server has
> to process GETATTR and SETATTR (and OPEN) operations that
> contain FATTR4_WORD1_TIME_CREATE as not an error.

Well, permissions or bad attribute values or other stuff may prevent
setting one of the attributes.

And setattr isn't guaranteed to be atomic, so I don't think you can
eliminate the possibility that part of it might succeed and part might
not.

But it might be more helpful to fail the whole thing up front if you
know part of it's going to fail?

> The protocol
> allows the server to indicate it ignored the time_create value
> by clearing the FATTR4_WORD1_TIME_CREATE bit in the attribute
> bitmask it returns in the reply.

Yes, I think you also return an error in that case, though.

--b.