[heads-up] mknod() broken on nfs4

[heads-up] mknod() broken on nfs4

[Al Viro]

	Try mknod(path, 0777, 0); with path leading into nfs4.  It
leads to call of nfs_open_create(), with nd->intent.open.file being
uninitialized.  Note that LOOKUP_CREATE is set and so's LOOKUP_EXCL,
but LOOKUP_OPEN isn't.  So nfs_atomic_lookup() falls through to
nfs_lookup(), which sees that we are doing exclusive create and just
does d_instantiate(dentry, NULL) and do nothing else.  And then
we hit ->create()...

	Results are ugly - random errors (often -EINVAL or -ENOENT)
and possibility of memory corruption if we manage to generate a request
that won't fail on server.  

	The really interesting question is what should we pass in
NFS_PROTO(dir)->create() in open_flags.  I suspect that you are
checking the wrong flag there (LOOKUP_CREATE instead of LOOKUP_OPEN),
but I'm not sure what *should* be passed when LOOKUP_OPEN is not
there...

Re: [heads-up] mknod() broken on nfs4

[Al Viro]

On Wed, Jun 22, 2011 at 12:59:00AM +0100, Al Viro wrote:
> 	Try mknod(path, 0777, 0); with path leading into nfs4.  It
> leads to call of nfs_open_create(), with nd->intent.open.file being
> uninitialized.  Note that LOOKUP_CREATE is set and so's LOOKUP_EXCL,
> but LOOKUP_OPEN isn't.  So nfs_atomic_lookup() falls through to
> nfs_lookup(), which sees that we are doing exclusive create and just
> does d_instantiate(dentry, NULL) and do nothing else.  And then
> we hit ->create()...
> 
> 	Results are ugly - random errors (often -EINVAL or -ENOENT)
> and possibility of memory corruption if we manage to generate a request
> that won't fail on server.  
> 
> 	The really interesting question is what should we pass in
> NFS_PROTO(dir)->create() in open_flags.  I suspect that you are
> checking the wrong flag there (LOOKUP_CREATE instead of LOOKUP_OPEN),
> but I'm not sure what *should* be passed when LOOKUP_OPEN is not
> there...

Argh...  Alas, it's not that simple.  Even though the code in nfs_open_create()
and nfs4_proc_create() seems to imply that passing NULL as ctx is OK and
expected, in reality that blows up since we end up with NULL cred passed
to nfs4_do_open(), which oopses on attempt to do get_rpccred(NULL) from
nfs4_get_state_owner().

Folks, how is that code supposed to work?  lookup_instantiate_filp() should
*not* be called by vfs_create() triggered by mknod().  And I don't see any
codepath in nfs_open_create() that would not step into that.  ctx == NULL
is the only thing that would skip it and it definitely isn't survivable
by nfs4_proc_create().  Moreover, we need the rpc_cred to come from somewhere
and nfs4_proc_create() needs to get it from us.

BTW, AFAICS fuse will oops in such situation as well...

Re: [heads-up] mknod() broken on nfs4

[Trond Myklebust]

On Wed, 2011-06-22 at 01:23 +0100, Al Viro wrote: 
> On Wed, Jun 22, 2011 at 12:59:00AM +0100, Al Viro wrote:
> > 	Try mknod(path, 0777, 0); with path leading into nfs4.  It
> > leads to call of nfs_open_create(), with nd->intent.open.file being
> > uninitialized.  Note that LOOKUP_CREATE is set and so's LOOKUP_EXCL,
> > but LOOKUP_OPEN isn't.  So nfs_atomic_lookup() falls through to
> > nfs_lookup(), which sees that we are doing exclusive create and just
> > does d_instantiate(dentry, NULL) and do nothing else.  And then
> > we hit ->create()...
> > 
> > 	Results are ugly - random errors (often -EINVAL or -ENOENT)
> > and possibility of memory corruption if we manage to generate a request
> > that won't fail on server.  
> > 
> > 	The really interesting question is what should we pass in
> > NFS_PROTO(dir)->create() in open_flags.  I suspect that you are
> > checking the wrong flag there (LOOKUP_CREATE instead of LOOKUP_OPEN),
> > but I'm not sure what *should* be passed when LOOKUP_OPEN is not
> > there...
> 
> Argh...  Alas, it's not that simple.  Even though the code in nfs_open_create()
> and nfs4_proc_create() seems to imply that passing NULL as ctx is OK and
> expected, in reality that blows up since we end up with NULL cred passed
> to nfs4_do_open(), which oopses on attempt to do get_rpccred(NULL) from
> nfs4_get_state_owner().
> 
> Folks, how is that code supposed to work?  lookup_instantiate_filp() should
> *not* be called by vfs_create() triggered by mknod().  And I don't see any
> codepath in nfs_open_create() that would not step into that.  ctx == NULL
> is the only thing that would skip it and it definitely isn't survivable
> by nfs4_proc_create().  Moreover, we need the rpc_cred to come from somewhere
> and nfs4_proc_create() needs to get it from us.

I agree that we should error out gracefully instead of blowing up, but I
fail to see why we want to support mknod for a regular file: it's not a
posix interface, nor is it substantially different from open(O_CREAT|
O_EXCL|O_NOFOLLOW). What is it's purpose?

> BTW, AFAICS fuse will oops in such situation as well...

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [heads-up] mknod() broken on nfs4

[Al Viro]

On Wed, Jun 22, 2011 at 07:00:20PM -0400, Trond Myklebust wrote:
> > Folks, how is that code supposed to work?  lookup_instantiate_filp() should
> > *not* be called by vfs_create() triggered by mknod().  And I don't see any
> > codepath in nfs_open_create() that would not step into that.  ctx == NULL
> > is the only thing that would skip it and it definitely isn't survivable
> > by nfs4_proc_create().  Moreover, we need the rpc_cred to come from somewhere
> > and nfs4_proc_create() needs to get it from us.
> 
> I agree that we should error out gracefully instead of blowing up, but I
> fail to see why we want to support mknod for a regular file: it's not a
> posix interface, nor is it substantially different from open(O_CREAT|
> O_EXCL|O_NOFOLLOW). What is it's purpose?

It always worked that way, all the way back to Unix v6 (and I'm fairly sure
to earlier than that; don't have v5 kernel source, unfortunately).  Worked
that way in Linux since 0.02/0.03/0.10, when Linus first added mknod(2)
(presumably 0.01 had been tested with /dev populated by Minix ;-)

As for POSIX, what it says is
     The only portable use of mknod() is to create a FIFO-special file.         
     If mode is not S_IFIFO or dev is not 0, the behaviour of mknod() is        
     unspecified.                                                               
and we support it for all non-directories.  Always had...

	Note that the right thing to do is to issue CLOSE and _not_ call
lookup_instantiate_filp() if we are called from sys_mknodat().  We don't
want to leak stateid...
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [heads-up] mknod() broken on nfs4

[Al Viro]

On Thu, Jun 23, 2011 at 12:19:46AM +0100, Al Viro wrote:

> It always worked that way, all the way back to Unix v6 (and I'm fairly sure
> to earlier than that; don't have v5 kernel source, unfortunately).  Worked
> that way in Linux since 0.02/0.03/0.10, when Linus first added mknod(2)
> (presumably 0.01 had been tested with /dev populated by Minix ;-)

After looking around on the net: in v3 kernel, ken/sys2.c:

mknod()
{
        int *ip;
        extern uchar;

        if(suser()) {
                ip = namei(&uchar, 1);
                if(ip != NULL) {
                        u.u_error = EEXIST;
                        goto out;
                }
        }
        if(u.u_error)
                return;
        ip = maknode(u.u_arg[1]);
        ip->i_addr[0] = u.u_arg[2];

out:
        iput(ip);
}

IOW, mknod(path, 0777, 0) will, indeed, create a regular file.  Root-only,
back then.

; ls -l ken/sys2.c 
-rw-r--r-- 1 al al 3060 Aug 30  1973 ken/sys2.c

v3 manpages don't mention mknod(2) at all; it *is* wired in syscall table
(syscall 14).  Section 2 manpages give syscall numbers; what they have for
#14 is makdir (not mentioned by that name in the kernel, described as
creating an empty directory with given pathname and mode sans . and ..
links - i.e. exactly what mknod(2) did all the way to v7 when given
S_IFDIR | something).

Re: [heads-up] mknod() broken on nfs4

[Al Viro]

On Thu, Jun 23, 2011 at 12:19:46AM +0100, Al Viro wrote:
> It always worked that way, all the way back to Unix v6 (and I'm fairly sure
> to earlier than that; don't have v5 kernel source, unfortunately).  Worked
> that way in Linux since 0.02/0.03/0.10, when Linus first added mknod(2)
> (presumably 0.01 had been tested with /dev populated by Minix ;-)
> 
> As for POSIX, what it says is
>      The only portable use of mknod() is to create a FIFO-special file.         
>      If mode is not S_IFIFO or dev is not 0, the behaviour of mknod() is        
>      unspecified.                                                               
> and we support it for all non-directories.  Always had...
> 
> 	Note that the right thing to do is to issue CLOSE and _not_ call
> lookup_instantiate_filp() if we are called from sys_mknodat().  We don't
> want to leak stateid...

OK...  See #untested in vfs-2.6.git; the last 5 commits there.  The actual
fixes are in the last one; to get rid of the dependency on the previous
we'd just need to pass open_flags as additional argument to
nameidata_to_nfs_open_context() (like untested@{1}, but without removal
of nameidata argument - that part depends on earlier commits and is, IMO,
a good idea anyway).

It seems to work here.  Testing and comments would be welcome...