diff mbox series

[1/1] NFSv4.1: fix zero value filehandle in post open getattr

Message ID 20230713195416.30414-1-olga.kornievskaia@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/1] NFSv4.1: fix zero value filehandle in post open getattr | expand

Commit Message

Olga Kornievskaia July 13, 2023, 7:54 p.m. UTC 
From: Olga Kornievskaia <kolga@netapp.com>

Currently, if the OPEN compound experiencing an error and needs to
get the file attributes separately, it will send a stand alone
GETATTR but it would use the filehandle from the results of
the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
is zero value. That generate a GETATTR that's sent with a zero
value filehandle, and results in the server returning an error.

Instead, for the CLAIM_FH OPEN, take the filehandle that was used
in the PUTFH of the OPEN compound.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Benjamin Coddington July 13, 2023, 9:08 p.m. UTC | #1 
On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:

> From: Olga Kornievskaia <kolga@netapp.com>
>
> Currently, if the OPEN compound experiencing an error and needs to
> get the file attributes separately, it will send a stand alone
> GETATTR but it would use the filehandle from the results of
> the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
> is zero value. That generate a GETATTR that's sent with a zero
> value filehandle, and results in the server returning an error.
>
> Instead, for the CLAIM_FH OPEN, take the filehandle that was used
> in the PUTFH of the OPEN compound.
>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8edc610dc1d3..0b1b49f01c5b 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendata *data,
>  			return status;
>  	}
>  	if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> +		struct nfs_fh *fh = &o_res->fh;
> +
>  		nfs4_sequence_free_slot(&o_res->seq_res);
> -		nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> +		if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> +			fh = NFS_FH(d_inode(data->dentry));
> +		nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
>  	}
>  	return 0;
>  }

Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
to fix the whitespace damage a few lines before this hunk too.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben
Benjamin Coddington July 13, 2023, 9:16 p.m. UTC | #2 
On 13 Jul 2023, at 17:08, Benjamin Coddington wrote:

> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>
>> From: Olga Kornievskaia <kolga@netapp.com>
>>
>> Currently, if the OPEN compound experiencing an error and needs to
>> get the file attributes separately, it will send a stand alone
>> GETATTR but it would use the filehandle from the results of
>> the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
>> is zero value. That generate a GETATTR that's sent with a zero
>> value filehandle, and results in the server returning an error.
>>
>> Instead, for the CLAIM_FH OPEN, take the filehandle that was used
>> in the PUTFH of the OPEN compound.
>>
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfs/nfs4proc.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 8edc610dc1d3..0b1b49f01c5b 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendata *data,
>>  			return status;
>>  	}
>>  	if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
>> +		struct nfs_fh *fh = &o_res->fh;
>> +
>>  		nfs4_sequence_free_slot(&o_res->seq_res);
>> -		nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
>> +		if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
>> +			fh = NFS_FH(d_inode(data->dentry));
>> +		nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
>>  	}
>>  	return 0;
>>  }
>
> Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
> to fix the whitespace damage a few lines before this hunk too.
>
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

.. and should we handle the other OPEN_CLAIM cases that use the filehandle
as well?

Ben
Olga Kornievskaia July 13, 2023, 9:27 p.m. UTC | #3 
On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Currently, if the OPEN compound experiencing an error and needs to
> > get the file attributes separately, it will send a stand alone
> > GETATTR but it would use the filehandle from the results of
> > the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
> > is zero value. That generate a GETATTR that's sent with a zero
> > value filehandle, and results in the server returning an error.
> >
> > Instead, for the CLAIM_FH OPEN, take the filehandle that was used
> > in the PUTFH of the OPEN compound.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4proc.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 8edc610dc1d3..0b1b49f01c5b 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendata *data,
> >                       return status;
> >       }
> >       if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> > +             struct nfs_fh *fh = &o_res->fh;
> > +
> >               nfs4_sequence_free_slot(&o_res->seq_res);
> > -             nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> > +             if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> > +                     fh = NFS_FH(d_inode(data->dentry));
> > +             nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
> >       }
> >       return 0;
> >  }
>
> Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
> to fix the whitespace damage a few lines before this hunk too.
>

I did try it first. I had 2 problems with it. First of o_arg->fh is a
"const struct" so it wouldn't allow me to be assigned without casting.
Ok so perhaps it's not a big deal that we are going against the
"const". Second of all, when I did do that, it produced the following
warning and so I thought perhaps I should really use the original fh
instead of what's in the args:

Jul 13 17:25:32 localhost kernel: ------------[ cut here ]------------
Jul 13 17:25:32 localhost kernel: WARNING: CPU: 0 PID: 5458 at
fs/nfs/nfs4xdr.c:978 encode_string+0x4c/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: Modules linked in: rpcsec_gss_krb5
auth_rpcgss nfsv4 dns_resolver nfs lockd grace uinput nft_fib_inet
nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4
nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack
nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr
vsock_loopback vmw_vsock_virtio_transport_common
vmw_vsock_vmci_transport vsock sunrpc vfat fat vmw_vmci xfs libcrc32c
vmwgfx drm_ttm_helper ttm drm_kms_helper nvme crct10dif_ce sr_mod
cdrom sg syscopyarea nvme_core sysfillrect sysimgblt ghash_ce sha2_ce
e1000e drm sha256_arm64 sha1_ce nvme_common fuse
Jul 13 17:25:32 localhost kernel: CPU: 0 PID: 5458 Comm: cat Kdump:
loaded Not tainted 6.4.0-rc7+ #11
Jul 13 17:25:32 localhost kernel: Hardware name: VMware, Inc.
VMware20,1/VBSA, BIOS VMW201.00V.20904234.BA64.2212051119 12/05/2022
Jul 13 17:25:32 localhost kernel: pstate: 21400005 (nzCv daif +PAN
-UAO -TCO +DIT -SSBS BTYPE=--)
Jul 13 17:25:32 localhost kernel: pc : encode_string+0x4c/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: lr : encode_string+0x30/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: sp : ffff80000ccab400
Jul 13 17:25:32 localhost kernel: x29: ffff80000ccab400 x28:
ffff00008cadf008 x27: ffff0000866f4e30
Jul 13 17:25:32 localhost kernel: x26: 0000000000440000 x25:
0000000000000000 x24: ffff80000949f008
Jul 13 17:25:32 localhost kernel: x23: ffff800001200c30 x22:
ffff00008a4ef830 x21: ffff00008cadf018
Jul 13 17:25:32 localhost kernel: x20: ffff00008cadf01a x19:
0000000000000858 x18: 0000000000000000
Jul 13 17:25:32 localhost kernel: x17: 0000000000000000 x16:
0000000000000000 x15: 0000000000000000
Jul 13 17:25:32 localhost kernel: x14: ffffffffffffffff x13:
0000000000000007 x12: 93449b0e64a317a5
Jul 13 17:25:32 localhost kernel: x11: 00000006e6fbcafb x10:
0000000000000e10 x9 : ffff800001212040
Jul 13 17:25:32 localhost kernel: x8 : ffff00008a4ef8e0 x7 :
0000000000000007 x6 : 93449b0e64a317a5
Jul 13 17:25:32 localhost kernel: x5 : ffff00008824b08c x4 :
ffff000082196008 x3 : ffff80000ccab4c0
Jul 13 17:25:32 localhost kernel: x2 : 000000000000021c x1 :
000000000000085c x0 : 0000000000000000
Jul 13 17:25:32 localhost kernel: Call trace:
Jul 13 17:25:32 localhost kernel: encode_string+0x4c/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_xdr_enc_getattr+0xb0/0x108 [nfsv4]
Jul 13 17:25:32 localhost kernel: rpcauth_wrap_req_encode+0x20/0x38 [sunrpc]
Jul 13 17:25:32 localhost kernel: rpcauth_wrap_req+0x24/0x38 [sunrpc]
Jul 13 17:25:32 localhost kernel: call_encode+0x180/0x258 [sunrpc]
Jul 13 17:25:32 localhost kernel: __rpc_execute+0xb8/0x3e0 [sunrpc]
Jul 13 17:25:32 localhost kernel: rpc_execute+0x160/0x1d8 [sunrpc]
Jul 13 17:25:32 localhost kernel: rpc_run_task+0x148/0x1f8 [sunrpc]
Jul 13 17:25:32 localhost kernel: nfs4_do_call_sync+0x78/0xc8 [nfsv4]
Jul 13 17:25:32 localhost kernel: _nfs4_proc_getattr+0xe8/0x120 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_proc_getattr+0x7c/0x140 [nfsv4]
Jul 13 17:25:32 localhost kernel: _nfs4_open_and_get_state+0x1d8/0x460 [nfsv4]
Jul 13 17:25:32 localhost kernel: _nfs4_do_open.isra.0+0x140/0x408 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_do_open+0x9c/0x238 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_atomic_open+0x100/0x118 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_file_open+0x11c/0x240 [nfsv4]
Jul 13 17:25:32 localhost kernel: do_dentry_open+0x140/0x490
Jul 13 17:25:32 localhost kernel: vfs_open+0x30/0x38
Jul 13 17:25:32 localhost kernel: do_open+0x14c/0x360
Jul 13 17:25:32 localhost kernel: path_openat+0x104/0x250
Jul 13 17:25:32 localhost kernel: do_filp_open+0x84/0x138
Jul 13 17:25:32 localhost kernel: do_sys_openat2+0xb4/0x170
Jul 13 17:25:32 localhost kernel: __arm64_sys_openat+0x64/0xb0
Jul 13 17:25:32 localhost kernel: invoke_syscall.constprop.0+0x7c/0xd0
Jul 13 17:25:32 localhost kernel: el0_svc_common.constprop.0+0x13c/0x158
Jul 13 17:25:32 localhost kernel: do_el0_svc+0x38/0xa8
Jul 13 17:25:32 localhost kernel: el0_svc+0x3c/0x198
Jul 13 17:25:32 localhost kernel: el0t_64_sync_handler+0xb4/0x130
Jul 13 17:25:32 localhost kernel: el0t_64_sync+0x17c/0x180
Jul 13 17:25:32 localhost kernel: ---[ end trace 0000000000000000 ]---


> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
>
> Ben
>
Olga Kornievskaia July 13, 2023, 9:32 p.m. UTC | #4 
On Thu, Jul 13, 2023 at 5:16 PM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 13 Jul 2023, at 17:08, Benjamin Coddington wrote:
>
> > On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
> >
> >> From: Olga Kornievskaia <kolga@netapp.com>
> >>
> >> Currently, if the OPEN compound experiencing an error and needs to
> >> get the file attributes separately, it will send a stand alone
> >> GETATTR but it would use the filehandle from the results of
> >> the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
> >> is zero value. That generate a GETATTR that's sent with a zero
> >> value filehandle, and results in the server returning an error.
> >>
> >> Instead, for the CLAIM_FH OPEN, take the filehandle that was used
> >> in the PUTFH of the OPEN compound.
> >>
> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> ---
> >>  fs/nfs/nfs4proc.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index 8edc610dc1d3..0b1b49f01c5b 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendata *data,
> >>                      return status;
> >>      }
> >>      if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> >> +            struct nfs_fh *fh = &o_res->fh;
> >> +
> >>              nfs4_sequence_free_slot(&o_res->seq_res);
> >> -            nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> >> +            if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> >> +                    fh = NFS_FH(d_inode(data->dentry));
> >> +            nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
> >>      }
> >>      return 0;
> >>  }
> >
> > Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
> > to fix the whitespace damage a few lines before this hunk too.
> >
> > Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
>
> .. and should we handle the other OPEN_CLAIM cases that use the filehandle
> as well?

I was pondering about that but I think the other OPEN CLAIM that use
the FH directly are recovery opens. I'm not sure if they need this.

>
> Ben
>
Benjamin Coddington July 13, 2023, 10:12 p.m. UTC | #5 
On 13 Jul 2023, at 17:27, Olga Kornievskaia wrote:

> On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <bcodding@redhat.com> wrote:
>>
>> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>>
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>
>>> Currently, if the OPEN compound experiencing an error and needs to
>>> get the file attributes separately, it will send a stand alone
>>> GETATTR but it would use the filehandle from the results of
>>> the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
>>> is zero value. That generate a GETATTR that's sent with a zero
>>> value filehandle, and results in the server returning an error.
>>>
>>> Instead, for the CLAIM_FH OPEN, take the filehandle that was used
>>> in the PUTFH of the OPEN compound.
>>>
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>>  fs/nfs/nfs4proc.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 8edc610dc1d3..0b1b49f01c5b 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendata *data,
>>>                       return status;
>>>       }
>>>       if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
>>> +             struct nfs_fh *fh = &o_res->fh;
>>> +
>>>               nfs4_sequence_free_slot(&o_res->seq_res);
>>> -             nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
>>> +             if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
>>> +                     fh = NFS_FH(d_inode(data->dentry));
>>> +             nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
>>>       }
>>>       return 0;
>>>  }
>>
>> Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
>> to fix the whitespace damage a few lines before this hunk too.
>>
>
> I did try it first. I had 2 problems with it. First of o_arg->fh is a
> "const struct" so it wouldn't allow me to be assigned without casting.
> Ok so perhaps it's not a big deal that we are going against the
> "const". Second of all, when I did do that, it produced the following
> warning and so I thought perhaps I should really use the original fh
> instead of what's in the args:

Oh maybe because this is the error path and nfs4_opendata is getting cleaned
up in nfs4_open_release()?  The comments in nfs4_open_release are a bit
confusing, but I think for the cases where we need to re-use the opendata we
are doing a kref_get on it.  Maybe we need a kref_get on the opendata for
this case?

.. I suspect we'd have the o_res.fh from nfs4_opendata_get_inode().  Out of
time, will check back in tomorrow.

Ben
Olga Kornievskaia July 14, 2023, 1:57 p.m. UTC | #6 
On Thu, Jul 13, 2023 at 6:13 PM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 13 Jul 2023, at 17:27, Olga Kornievskaia wrote:
>
> > On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <bcodding@redhat.com> wrote:
> >>
> >> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
> >>
> >>> From: Olga Kornievskaia <kolga@netapp.com>
> >>>
> >>> Currently, if the OPEN compound experiencing an error and needs to
> >>> get the file attributes separately, it will send a stand alone
> >>> GETATTR but it would use the filehandle from the results of
> >>> the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
> >>> is zero value. That generate a GETATTR that's sent with a zero
> >>> value filehandle, and results in the server returning an error.
> >>>
> >>> Instead, for the CLAIM_FH OPEN, take the filehandle that was used
> >>> in the PUTFH of the OPEN compound.
> >>>
> >>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >>> ---
> >>>  fs/nfs/nfs4proc.c | 6 +++++-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >>> index 8edc610dc1d3..0b1b49f01c5b 100644
> >>> --- a/fs/nfs/nfs4proc.c
> >>> +++ b/fs/nfs/nfs4proc.c
> >>> @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendata *data,
> >>>                       return status;
> >>>       }
> >>>       if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> >>> +             struct nfs_fh *fh = &o_res->fh;
> >>> +
> >>>               nfs4_sequence_free_slot(&o_res->seq_res);
> >>> -             nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> >>> +             if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> >>> +                     fh = NFS_FH(d_inode(data->dentry));
> >>> +             nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
> >>>       }
> >>>       return 0;
> >>>  }
> >>
> >> Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
> >> to fix the whitespace damage a few lines before this hunk too.
> >>
> >
> > I did try it first. I had 2 problems with it. First of o_arg->fh is a
> > "const struct" so it wouldn't allow me to be assigned without casting.
> > Ok so perhaps it's not a big deal that we are going against the
> > "const". Second of all, when I did do that, it produced the following
> > warning and so I thought perhaps I should really use the original fh
> > instead of what's in the args:
>
> Oh maybe because this is the error path and nfs4_opendata is getting cleaned
> up in nfs4_open_release()?  The comments in nfs4_open_release are a bit
> confusing, but I think for the cases where we need to re-use the opendata we
> are doing a kref_get on it.  Maybe we need a kref_get on the opendata for
> this case?

I guess I don't understand what's the problem with the current
approach which is simple.

> .. I suspect we'd have the o_res.fh from nfs4_opendata_get_inode().  Out of
> time, will check back in tomorrow.
>
> Ben
>
Benjamin Coddington July 14, 2023, 3:40 p.m. UTC | #7 
On 14 Jul 2023, at 9:57, Olga Kornievskaia wrote:
>
> I guess I don't understand what's the problem with the current
> approach which is simple.

No problem - I'm just bad at code review.

I was trying to get my head around why the o_arg->fh couldn't be used, and
then wondering why you were hitting that warning.

Then I worried that the opendata was already freed at the point the getattr
was trying to use it again, and because its a codepath I can't exercise
easily my normal approach of just throwing in debug statements didn't work -
but I think the opendata is still there because _nfs4_do_open has it pinned.

Ben
diff mbox series

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8edc610dc1d3..0b1b49f01c5b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2703,8 +2703,12 @@  static int _nfs4_proc_open(struct nfs4_opendata *data,
 			return status;
 	}
 	if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
+		struct nfs_fh *fh = &o_res->fh;
+
 		nfs4_sequence_free_slot(&o_res->seq_res);
-		nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
+		if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
+			fh = NFS_FH(d_inode(data->dentry));
+		nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
 	}
 	return 0;
 }