Re: [PATCH v2] NFSv4: Save the owner/group name string when doing open

Re: [PATCH v2] NFSv4: Save the owner/group name string when doing open

[Jonathan Nieder]

Hi,

Trond Myklebust wrote:

> [Subject: NFSv4: Save the owner/group name string when doing open]
>
> ...so that we can do the uid/gid mapping outside the asynchronous RPC
> context.
> This fixes a bug in the current NFSv4 atomic open code where the client
> isn't able to determine what the true uid/gid fields of the file are,
> (because the asynchronous nature of the OPEN call denies it the ability
> to do an upcall) and so fills them with default values, marking the
> inode as needing revalidation.
> Unfortunately, in some cases, the VFS will do some additional sanity
> checks on the file, and may override the server's decision to allow
> the open because it sees the wrong owner/group fields.

Thanks!  This patch (commit 6926afd1925a, 2012-01-07) fixes the
following client-side bug[1]:

| Our home directories here are mounted over NFS4. When I log in to machine A
| and run
|
| vim
| :q
|
| and then log into machine B and do:
|
| vim
| :q
|
| I get E137: Viminfo file is not writable: /users/system/rtheys/.viminfo
|
| Every invocation of 'vim and :q' will trigger this.
|
| Explicitely doing a stat of the file fixes this.

Rik Theys bisected and found the bug reproducible after and not before
v2.6.32-rc1~412^2~48^2~15 (NFSv4: Don't do idmapper upcalls for
asynchronous RPC calls, 2009-08-09).

[...]
>  6 files changed, 162 insertions(+), 64 deletions(-)

Now I am wondering what, if anything, can be done to fix this in the
2.6.32.y, 3.0.y, and 3.2.y stable kernels.  The patch looks too big
for inclusion under the usual stable_kernel_rules:

 - It cannot be bigger than 100 lines, with context.

Ideas?
Jonathan

[1] http://bugs.debian.org/659111
    http://thread.gmane.org/gmane.linux.nfs/37230

Re: [PATCH v2] NFSv4: Save the owner/group name string when doing open

[Jonathan Nieder]

Jonathan Nieder wrote:

[...]
> | and then log into machine B and do:
> |
> | vim
> | :q
> |
> | I get E137: Viminfo file is not writable: /users/system/rtheys/.viminfo
> |
> | Every invocation of 'vim and :q' will trigger this.
> |
> | Explicitely doing a stat of the file fixes this.
>
> Rik Theys bisected and found the bug reproducible after and not before
> v2.6.32-rc1~412^2~48^2~15 (NFSv4: Don't do idmapper upcalls for
> asynchronous RPC calls, 2009-08-09).
>
> [...]
>>  6 files changed, 162 insertions(+), 64 deletions(-)
>
> Now I am wondering what, if anything, can be done to fix this in the
> 2.6.32.y, 3.0.y, and 3.2.y stable kernels.  The patch looks too big
> for inclusion under the usual stable_kernel_rules:

Trond had a neat idea for fixing this.  Let's see how easy it is for a
novice like me to understand.

As explained at [1], the problem is that after the OPEN call .viminfo
has the default values for st_uid and st_gid cached (i.e., 0xfffffffe)
because it does not want to let rpciod wait during an idmapper upcall
to fill them in.  The fix used in mainline is to save the owner and
group as strings and perform the upcall in _nfs4_proc_open outside the
rpciod context.

The fix for stable kernels that Trond suggests is to notice when
st_uid and st_gid have not been filled in and perform a separate
GETATTR call.  The patch is nice and small.  My (ignorant) worry: does
nfs4_open_reclaim need the same fix?

[1] http://thread.gmane.org/gmane.linux.nfs/37230/focus=37250
---
Rik, results from testing would be interesting if you have a chance to
try it.  Thanks again for your help, all.

Jonathan

 fs/nfs/nfs4proc.c |    1 +
 1 file changed, 1 insertion(+)

diff --git i/fs/nfs/nfs4proc.c w/fs/nfs/nfs4proc.c
index 3d6730213f9d..30f6548f2b99 100644
--- i/fs/nfs/nfs4proc.c
+++ w/fs/nfs/nfs4proc.c
@@ -1771,6 +1771,7 @@ static int _nfs4_do_open(struct inode *dir, struct path *path, fmode_t fmode, in
 			nfs_setattr_update_inode(state->inode, sattr);
 		nfs_post_op_update_inode(state->inode, opendata->o_res.f_attr);
 	}
+	nfs_revalidate_inode(server, state->inode);
 	nfs4_opendata_put(opendata);
 	nfs4_put_state_owner(sp);
 	*res = state;
--