Re: [PATCH] NFSv4: fix open(O_RDONLY|O_TRUNC) returning EBADF

[Nadav Shemer <nadav@tonian.com>]

On Fri, Jul 19, 2013 at 5:21 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 11 Jul 2013 10:49:18 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
>
>> On Tue,  9 Jul 2013 19:16:45 +0300
>> Nadav Shemer <nadav@tonian.com> wrote:
>>
>> > Move ATTR_OPEN handling from nfs4_proc_setattr into nfs4_do_setattr
>> >
>> > Signed-off-by: Nadav Shemer <nadav@tonian.com>
>> > ---
>> > Hello.
>> >
>> > I've come across an oddity while testing filesystem coverage
>> > My test creates a non-empty file without write permissions and tries to open it with O_RDONLY|O_TRUNC
>> > It expects EACCES (and gets that for local filesystems and NFSv3) but gets EBADF on NFSv4 (and v4.1)
>> >
>> > I found some history on this: In a previous kernel it would just hang due to mishandling the NFS4ERR_OPENMODE exception
>> > http://www.spinics.net/lists/linux-nfs/msg28881.html
>> >
>> > A fix for this was introduced (it specifically tests for NFS4ERR_OPENMODE and returns EACCES for the open() case, EBADF otherwise)
>> > http://www.spinics.net/linux/fedora/fedora-kernel/msg03736.html
>> > but another patch was also introduced in the same set which seems to break it (it optimizes away the time modification and removes ATTR_OPEN in nfs4_proc_setattr)
>> > http://www.spinics.net/linux/fedora/fedora-kernel/msg03732.html
>> >
>> > By moving the 'Deal with open(O_TRUNC)' bit inside (into nfs4_do_setattr), I got it working again (with no other functional change, as far as I can see)
>> >
>> >  fs/nfs/nfs4proc.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> > index 8fbc100..17b9f32 100644
>> > --- a/fs/nfs/nfs4proc.c
>> > +++ b/fs/nfs/nfs4proc.c
>> > @@ -2180,6 +2180,10 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
>> >             .inode = inode,
>> >     };
>> >     int err;
>> > +   int is_o_trunc = sattr->ia_valid & ATTR_OPEN;
>> > +   /* Deal with open(O_TRUNC) */
>> > +   if (sattr->ia_valid & ATTR_OPEN)
>> > +           sattr->ia_valid &= ~(ATTR_MTIME|ATTR_CTIME|ATTR_OPEN);
>>
>> I'm not sure you really need to move the above if statement into this
>> function, do you? I think it'd be best to leave that where it is and
>> just add the is_o_trunc variable the special handling for it below.
>>
>
> Actually, disregard that comment. I was confused as to which function
> calls which here...
>
> Still, I think we'd do better to keep this logic consolidated in
> nfs4_proc_setattr instead of spreading it around. I'm a little
> concerned that this may change the behavior in the open codepath, which
> also calls nfs4_do_setattr. More below...
>
>> >     do {
>> >             err = _nfs4_do_setattr(inode, cred, fattr, sattr, state);
>> >             switch (err) {
>> > @@ -2193,7 +2197,7 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
>> >                     }
>> >                     if (state && !(state->state & FMODE_WRITE)) {
>> >                             err = -EBADF;
>> > -                           if (sattr->ia_valid & ATTR_OPEN)
>> > +                           if (is_o_trunc)
>> >                                     err = -EACCES;
>> >                             goto out;
>> >                     }
>> > @@ -2774,10 +2778,6 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
>> >
>> >     nfs_fattr_init(fattr);
>> >
>> > -   /* Deal with open(O_TRUNC) */
>> > -   if (sattr->ia_valid & ATTR_OPEN)
>> > -           sattr->ia_valid &= ~(ATTR_MTIME|ATTR_CTIME|ATTR_OPEN);
>> > -
>> >     /* Optimization: if the end result is no change, don't RPC */
>> >     if ((sattr->ia_valid & ~(ATTR_FILE)) == 0)
>> >             return 0;
>>
>
> It looks like we're just clearing ATTR_OPEN here in order to make the
> check that follows that happy. Would it make more sense to instead
> leave ATTR_OPEN set in this place and fix that check to ignore
> ATTR_OPEN?
Yes, it does :)
My patch broke that check, and your suggestion also makes the patch
much smaller (resending it)

This is also the same check that's in nfs/inode.c (which is the only
caller I could find), but this one happens after removing
ATTR_MTIME|ATTR_CTIME (nfs/inode.c only removed ATTR_SIZE and non-NFS
attributes)

>
> --
> Jeff Layton <jlayton@redhat.com>