linux-kernel - Re: [PATCH 01/19] fs: new API for handling inode->i

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: <87indanv2m.fsf@notabene.neil.brown.name>
Date:   Thu, 14 Dec 2017 09:04:33 +1100
From:   NeilBrown <neilb@...e.com>
To:     Jeff Layton <jlayton@...nel.org>, linux-fsdevel@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, hch@....de, neilb@...e.de,
        bfields@...ldses.org, amir73il@...il.com, jack@...e.de,
        viro@...iv.linux.org.uk
Subject: Re: [PATCH 01/19] fs: new API for handling inode->i_version

On Wed, Dec 13 2017, Jeff Layton wrote:

> +/*
> + * The change attribute (i_version) is mandated by NFSv4 and is mostly for
> + * knfsd, but is also used for other purposes (e.g. IMA). The i_version must
> + * appear different to observers if there was a change to the inode's data or
> + * metadata since it was last queried.
> + *
> + * It should be considered an opaque value by observers. If it remains the same
> + * since it was last checked, then nothing has changed in the inode. If it's
> + * different then something has changed. Observers cannot infer anything about
> + * the nature or magnitude of the changes from the value, only that the inode
> + * has changed in some fashion.

I agree that it "should be" considered opaque, but I have a suspicion
that NFSv4 doesn't consider it opaque.
There is something about write delegations and the server performing a
GETATTR callback to the delegated client so that it can answer GETATTR
from other clients without recalling the delegation.

Specifically section "10.4.3 Handling of CB_GETATTR" of RFC5661 contains
the text:

   o  The client will create a value greater than c that will be used
      for communicating that modified data is held at the client.  Let
      this value be represented by d.

"c" here is a 'change' attribute.

Then:

   While the change attribute is opaque to the client in the sense that
   it has no idea what units of time, if any, the server is counting
   change with, it is not opaque in that the client has to treat it as
   an unsigned integer, and the server has to be able to see the results
   of the client's changes to that integer.  Therefore, the server MUST
   encode the change attribute in network order when sending it to the
   client.  The client MUST decode it from network order to its native
   order when receiving it, and the client MUST encode it in network
   order when sending it to the server.  For this reason, change is
   defined as an unsigned integer rather than an opaque array of bytes.

This all suggests that nfsd needs to be certain that "incrementing" the
change id will produce a new changeid, which has not been used before,
and also suggests that nfsd needs to be able to control the changeid
stored after writes that result from a delegation being returned.

I'd just like to say that this is one of the most annoying dumb features
of NFSv4, because it is trivial to fix and I suggested a fix before
NFSv4.0 was finalized.  Grumble.

Otherwise the patch set looks good.  I haven't gone over the code
closely, the but approach is spot-on.

NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)