Re: [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

Re: [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Eric Van Hensbergen]

[-- Attachment #1: Type: text/plain, Size: 4064 bytes --]

I've done some digging from the client side.  As is mentioned in Miklos'
original patch (which appears to have been shot down) the higher level
implementation appear to be broken for this.  Here's what the code looks
like for fstat in fs/stat.c:

int vfs_fstat(unsigned int fd, struct kstat *stat)
{
        struct fd f = fdget_raw(fd);
        int error = -EBADF;

        if (f.file) {
                error = vfs_getattr(&f.file->f_path, stat);
                fdput(f);
        }
        return error;
}

In other words, it only uses the open fd to derrive a path and then
executes the getattr off of that path.  If that path no longer exists
(because of unlink or remove) then you are hosed.  In my understanding, the
"work around" I suppose is the so-called 'silly renaming' where
remove/unlink simply does a rename until all open instances are closed.

The frustrating thing is that the 9p protocol is setup to work off of the
fid, which maps to the fd -- so its perfectly capable of the original
semantic but the high level code in fs/stat.c only wants to give a path.

I can do a work around on the client where a getattr "favors" open fids for
the operation or I can do the sillyrename hack that NFS and CIFS has used
but both of those look god-awful.

     -eric




On Fri, Apr 10, 2015 at 7:30 AM, Mark Glines <mark@glines.org> wrote:

> This bug makes it difficult to run a Debian Jessie guest with a 9pfs
> root filesystem, because "apt-get update" uses the open-unlink-fstat
> idiom.  With this bug present, apt dies with the following error:
>
> E: Unable to determine file size for fd 7 - fstat (2: No such file or
> directory)
>
> --
> You received this bug notification because you are a member of qemu-
> devel-ml, which is subscribed to QEMU.
> https://bugs.launchpad.net/bugs/1336794
>
> Title:
>   9pfs does not honor open file handles on unlinked files
>
> Status in QEMU:
>   New
>
> Bug description:
>   This was originally filed over here:
>   https://bugzilla.redhat.com/show_bug.cgi?id=1114221
>
>   The open-unlink-fstat idiom used in some places to create an anonymous
>   private temporary file does not work in a QEMU guest over a virtio-9p
>   filesystem.
>
>   Version-Release number of selected component (if applicable):
>
>   qemu-kvm-1.6.2-6.fc20.x86_64
>   qemu-system-x86-1.6.2-6.fc20.x86_64
>   (those are fedora RPMs)
>
>   How reproducible:
>
>   Always. See this example C program:
>
>   https://bugzilla.redhat.com/attachment.cgi?id=913069
>
>   Steps to Reproduce:
>   1. Export a filesystem with virt-manager for the guest.
>         (type: mount, driver: default, mode: passthrough)
>   2. Start guest and mount that filesystem
>         (mount -t 9p -o trans=virtio,version=9p2000.L  ...)
>   3. Run a program that uses open-unlink-fstat
>         (in my case it was trying to compile Perl 5.20)
>
>   Actual results:
>
>   fstat fails:
>
>   open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
>   unlink("/home/tst/filename")            = 0
>   fstat(3, 0x23aa1a8)                     = -1 ENOENT (No such file or
> directory)
>   close(3)
>
>   Expected results:
>
>   open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
>   unlink("/home/tst/filename")            = 0
>   fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
>   fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
>   close(3)
>
>   Additional info:
>
>   There was a patch put into the kernel back in '07 to handle this very
>   problem for other filesystems; maybe its helpful:
>
>         http://lwn.net/Articles/251228/
>
>   There is also a thread on LKML from last December specifically about
>   this very problem:
>
>         https://lkml.org/lkml/2013/12/31/163
>
>   There was a discussion on the QEMU list back in '11 that doesn't seem
>   to have come to a conclusion, but did provide the test program that
>   i've attached to this report:
>
>         http://marc.info/?l=qemu-devel&m=130443605720648&w=2
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/qemu/+bug/1336794/+subscriptions
>
>

[-- Attachment #2: Type: text/html, Size: 5761 bytes --]

Re: [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Dominique Martinet]

Hi,

for what it's worth, the sample code given works with nfs-ganesha
server, so I'm not sure what's not working here.

Here's the server traces:
TATTACH: tag=1 fid=0 afid=-1 uname='nobody' aname='/export' n_uname=-1
RATTACH: tag=1 fid=0 qid=(type=128,version=0,path=1)
TGETATTR: tag=1 fid=0 request_mask=0x7ff
RGETATTR: tag=1 valid=0x7ff qid=(type=128,version=0,path=1) mode=040755 uid=0 gid=0 nlink=3 rdev=192 size=4096 blksize=4096 blocks=8 atime=(1428909387,0) mtime=(1428909389,0) ctime=(1428909389,0) btime=(0,0) gen=0, data_version=0
TATTACH: tag=1 fid=1 afid=-1 uname='' aname='/export' n_uname=0
RATTACH: tag=1 fid=1 qid=(type=128,version=0,path=1)
TGETATTR: tag=1 fid=1 request_mask=0x3fff
RGETATTR: tag=1 valid=0x7ff qid=(type=128,version=0,path=1) mode=040755 uid=0 gid=0 nlink=3 rdev=192 size=4096 blksize=4096 blocks=8 atime=(1428909387,0) mtime=(1428909389,0) ctime=(1428909389,0) btime=(0,0) gen=0, data_version=0
TWALK: tag=1 fid=1 newfid=2 nwname=1 (component 1/1 :test.txt)
RERROR(_9P_TWALK) tag=1 err=(2|No such file or directory)
TWALK: tag=1 fid=1 newfid=2 nwname=0
RWALK: tag=1 fid=1 newfid=2 nwqid=0 fileid=1 pentry=0x8278c0 refcount=4
TLCREATE: tag=1 fid=2 name=test.txt flags=0100102 mode=0100644 gid=0
RLCREATE: tag=1 fid=2 name=test.txt qid=(type=0,version=0,path=144115205255725065) iounit=0 pentry=0x7fffc0000d00
TWALK: tag=1 fid=1 newfid=3 nwname=1 (component 1/1 :test.txt)
RWALK: tag=1 fid=1 newfid=3 nwqid=1 fileid=144115205255725065 pentry=0x7fffc0000d00 refcount=3
TGETATTR: tag=1 fid=3 request_mask=0x17ff
RGETATTR: tag=1 valid=0x7ff qid=(type=0,version=0,path=144115205255725065) mode=0100644 uid=0 gid=0 nlink=1 rdev=192 size=0 blksize=4096 blocks=0 atime=(1428909674,0) mtime=(1428909674,0) ctime=(1428909674,0) btime=(0,0) gen=0, data_version=0
TWRITE: tag=1 fid=2 offset=0 count=6
RWRITE: tag=1 fid=2 offset=0 input count=6 output count=6
TUNLINKAT: tag=1 dfid=1 name=test.txt
TUNLINKAT: tag=1 dfid=1 name=test.txt
TGETATTR: tag=1 fid=3 request_mask=0x3fff
RGETATTR: tag=1 valid=0x7ff qid=(type=0,version=0,path=144115205255725065) mode=0100644 uid=0 gid=0 nlink=0 rdev=192 size=6 blksize=4096 blocks=0 atime=(1428909674,0) mtime=(1428909674,0) ctime=(1428909674,0) btime=(0,0) gen=0, data_version=0
TCLUNK: tag=1 fid=2
RCLUNK: tag=1 fid=2
TCLUNK: tag=1 fid=3
RCLUNK: tag=1 fid=3

(if you're not familiar with 9P, ATTACH = mount, WALK = create a new
'fid' either clone of current file (nwname = 0) or lookup, CLUNK ~=
close. Rest is obvious enough)


There's no lookup between the unlink and the getattr, so what I think is
missing is that both qemu and diod do not understand that fids 2 and 3
refer to the same open file ?
It's a bit of a weird behavior that the client will open a new fid
through lookup walk for a first stat, but I'm mounting with cache=none
here so you should be having the same.

-- 
Dominique

Re: [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Eric Van Hensbergen]

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

Well, technically fid 3 isn't 'open', only fid 2 is open - at least
according to the protocol.  fid 3 and fid 2 are both clones of fid 1.
However, thanks for the alternative workaround.  If you get a chance, can
you check that my change to the client to partially fix this for the other
servers doesn't break nfs-ganesha:

https://github.com/ericvh/linux/commit/fddc7721d6d19e4e6be4905f37ade5b0521f4ed5

On Mon, Apr 13, 2015 at 3:27 AM, Dominique Martinet <
dominique.martinet@cea.fr> wrote:

> Hi,
>
> for what it's worth, the sample code given works with nfs-ganesha
> server, so I'm not sure what's not working here.
>
> Here's the server traces:
> TATTACH: tag=1 fid=0 afid=-1 uname='nobody' aname='/export' n_uname=-1
> RATTACH: tag=1 fid=0 qid=(type=128,version=0,path=1)
> TGETATTR: tag=1 fid=0 request_mask=0x7ff
> RGETATTR: tag=1 valid=0x7ff qid=(type=128,version=0,path=1) mode=040755
> uid=0 gid=0 nlink=3 rdev=192 size=4096 blksize=4096 blocks=8
> atime=(1428909387,0) mtime=(1428909389,0) ctime=(1428909389,0) btime=(0,0)
> gen=0, data_version=0
> TATTACH: tag=1 fid=1 afid=-1 uname='' aname='/export' n_uname=0
> RATTACH: tag=1 fid=1 qid=(type=128,version=0,path=1)
> TGETATTR: tag=1 fid=1 request_mask=0x3fff
> RGETATTR: tag=1 valid=0x7ff qid=(type=128,version=0,path=1) mode=040755
> uid=0 gid=0 nlink=3 rdev=192 size=4096 blksize=4096 blocks=8
> atime=(1428909387,0) mtime=(1428909389,0) ctime=(1428909389,0) btime=(0,0)
> gen=0, data_version=0
> TWALK: tag=1 fid=1 newfid=2 nwname=1 (component 1/1 :test.txt)
> RERROR(_9P_TWALK) tag=1 err=(2|No such file or directory)
> TWALK: tag=1 fid=1 newfid=2 nwname=0
> RWALK: tag=1 fid=1 newfid=2 nwqid=0 fileid=1 pentry=0x8278c0 refcount=4
> TLCREATE: tag=1 fid=2 name=test.txt flags=0100102 mode=0100644 gid=0
> RLCREATE: tag=1 fid=2 name=test.txt
> qid=(type=0,version=0,path=144115205255725065) iounit=0
> pentry=0x7fffc0000d00
> TWALK: tag=1 fid=1 newfid=3 nwname=1 (component 1/1 :test.txt)
> RWALK: tag=1 fid=1 newfid=3 nwqid=1 fileid=144115205255725065
> pentry=0x7fffc0000d00 refcount=3
> TGETATTR: tag=1 fid=3 request_mask=0x17ff
> RGETATTR: tag=1 valid=0x7ff qid=(type=0,version=0,path=144115205255725065)
> mode=0100644 uid=0 gid=0 nlink=1 rdev=192 size=0 blksize=4096 blocks=0
> atime=(1428909674,0) mtime=(1428909674,0) ctime=(1428909674,0) btime=(0,0)
> gen=0, data_version=0
> TWRITE: tag=1 fid=2 offset=0 count=6
> RWRITE: tag=1 fid=2 offset=0 input count=6 output count=6
> TUNLINKAT: tag=1 dfid=1 name=test.txt
> TUNLINKAT: tag=1 dfid=1 name=test.txt
> TGETATTR: tag=1 fid=3 request_mask=0x3fff
> RGETATTR: tag=1 valid=0x7ff qid=(type=0,version=0,path=144115205255725065)
> mode=0100644 uid=0 gid=0 nlink=0 rdev=192 size=6 blksize=4096 blocks=0
> atime=(1428909674,0) mtime=(1428909674,0) ctime=(1428909674,0) btime=(0,0)
> gen=0, data_version=0
> TCLUNK: tag=1 fid=2
> RCLUNK: tag=1 fid=2
> TCLUNK: tag=1 fid=3
> RCLUNK: tag=1 fid=3
>
> (if you're not familiar with 9P, ATTACH = mount, WALK = create a new
> 'fid' either clone of current file (nwname = 0) or lookup, CLUNK ~=
> close. Rest is obvious enough)
>
>
> There's no lookup between the unlink and the getattr, so what I think is
> missing is that both qemu and diod do not understand that fids 2 and 3
> refer to the same open file ?
> It's a bit of a weird behavior that the client will open a new fid
> through lookup walk for a first stat, but I'm mounting with cache=none
> here so you should be having the same.
>
> --
> Dominique
>

[-- Attachment #2: Type: text/html, Size: 4145 bytes --]

Re: [Qemu-devel] [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Al Viro]

On Mon, Apr 13, 2015 at 04:05:28PM +0000, Eric Van Hensbergen wrote:
> Well, technically fid 3 isn't 'open', only fid 2 is open - at least
> according to the protocol.  fid 3 and fid 2 are both clones of fid 1.
> However, thanks for the alternative workaround.  If you get a chance, can
> you check that my change to the client to partially fix this for the other
> servers doesn't break nfs-ganesha:
> 
> https://github.com/ericvh/linux/commit/fddc7721d6d19e4e6be4905f37ade5b0521f4ed5

BTW, what the hell is going on in v9fs_vfs_mknod() and v9fs_vfs_link()?
You allocate 4Kb buffer, sprintf into it ("b %u %u", "c %u %u", or "%d\n")
feed it to v9fs_vfs_mkspecial() and immediately free it.  What's wrong with
a local array of char?  In the worst case it needs to be char name[24] -
surely, we are not so tight on stack that extra 16 bytes (that array instead
of a pointer) would drive us over the cliff?

IOW, do you have any problem with this:
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 703342e..cda68f7 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1370,6 +1370,8 @@ v9fs_vfs_symlink(struct inode *dir, struct dentry *dentry, const char *symname)
 	return v9fs_vfs_mkspecial(dir, dentry, P9_DMSYMLINK, symname);
 }
 
+#define U32_MAX_DIGITS 10
+
 /**
  * v9fs_vfs_link - create a hardlink
  * @old_dentry: dentry for file to link to
@@ -1383,7 +1385,7 @@ v9fs_vfs_link(struct dentry *old_dentry, struct inode *dir,
 	      struct dentry *dentry)
 {
 	int retval;
-	char *name;
+	char name[1 + U32_MAX_DIGITS + 2]; /* sign + number + \n + \0 */
 	struct p9_fid *oldfid;
 
 	p9_debug(P9_DEBUG_VFS, " %lu,%pd,%pd\n",
@@ -1393,20 +1395,12 @@ v9fs_vfs_link(struct dentry *old_dentry, struct inode *dir,
 	if (IS_ERR(oldfid))
 		return PTR_ERR(oldfid);
 
-	name = __getname();
-	if (unlikely(!name)) {
-		retval = -ENOMEM;
-		goto clunk_fid;
-	}
-
 	sprintf(name, "%d\n", oldfid->fid);
 	retval = v9fs_vfs_mkspecial(dir, dentry, P9_DMLINK, name);
-	__putname(name);
 	if (!retval) {
 		v9fs_refresh_inode(oldfid, d_inode(old_dentry));
 		v9fs_invalidate_inode_attr(dir);
 	}
-clunk_fid:
 	p9_client_clunk(oldfid);
 	return retval;
 }
@@ -1425,7 +1419,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t rde
 {
 	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(dir);
 	int retval;
-	char *name;
+	char name[2 + U32_MAX_DIGITS + 1 + U32_MAX_DIGITS + 1];
 	u32 perm;
 
 	p9_debug(P9_DEBUG_VFS, " %lu,%pd mode: %hx MAJOR: %u MINOR: %u\n",
@@ -1435,26 +1429,16 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t rde
 	if (!new_valid_dev(rdev))
 		return -EINVAL;
 
-	name = __getname();
-	if (!name)
-		return -ENOMEM;
 	/* build extension */
 	if (S_ISBLK(mode))
 		sprintf(name, "b %u %u", MAJOR(rdev), MINOR(rdev));
 	else if (S_ISCHR(mode))
 		sprintf(name, "c %u %u", MAJOR(rdev), MINOR(rdev));
-	else if (S_ISFIFO(mode))
-		*name = 0;
-	else if (S_ISSOCK(mode))
+	else
 		*name = 0;
-	else {
-		__putname(name);
-		return -EINVAL;
-	}
 
 	perm = unixmode2p9mode(v9ses, mode);
 	retval = v9fs_vfs_mkspecial(dir, dentry, perm, name);
-	__putname(name);
 
 	return retval;
 }

Re: [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Eric Van Hensbergen]

[-- Attachment #1: Type: text/plain, Size: 3906 bytes --]

That patch looks fine by me.  Happy to put it in the queue.  Thanks Al.

On Tue, Apr 14, 2015 at 11:07 AM Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Mon, Apr 13, 2015 at 04:05:28PM +0000, Eric Van Hensbergen wrote:
> > Well, technically fid 3 isn't 'open', only fid 2 is open - at least
> > according to the protocol.  fid 3 and fid 2 are both clones of fid 1.
> > However, thanks for the alternative workaround.  If you get a chance, can
> > you check that my change to the client to partially fix this for the
> other
> > servers doesn't break nfs-ganesha:
> >
> >
> https://github.com/ericvh/linux/commit/fddc7721d6d19e4e6be4905f37ade5b0521f4ed5
>
> BTW, what the hell is going on in v9fs_vfs_mknod() and v9fs_vfs_link()?
> You allocate 4Kb buffer, sprintf into it ("b %u %u", "c %u %u", or "%d\n")
> feed it to v9fs_vfs_mkspecial() and immediately free it.  What's wrong with
> a local array of char?  In the worst case it needs to be char name[24] -
> surely, we are not so tight on stack that extra 16 bytes (that array
> instead
> of a pointer) would drive us over the cliff?
>
> IOW, do you have any problem with this:
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 703342e..cda68f7 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1370,6 +1370,8 @@ v9fs_vfs_symlink(struct inode *dir, struct dentry
> *dentry, const char *symname)
>         return v9fs_vfs_mkspecial(dir, dentry, P9_DMSYMLINK, symname);
>  }
>
> +#define U32_MAX_DIGITS 10
> +
>  /**
>   * v9fs_vfs_link - create a hardlink
>   * @old_dentry: dentry for file to link to
> @@ -1383,7 +1385,7 @@ v9fs_vfs_link(struct dentry *old_dentry, struct
> inode *dir,
>               struct dentry *dentry)
>  {
>         int retval;
> -       char *name;
> +       char name[1 + U32_MAX_DIGITS + 2]; /* sign + number + \n + \0 */
>         struct p9_fid *oldfid;
>
>         p9_debug(P9_DEBUG_VFS, " %lu,%pd,%pd\n",
> @@ -1393,20 +1395,12 @@ v9fs_vfs_link(struct dentry *old_dentry, struct
> inode *dir,
>         if (IS_ERR(oldfid))
>                 return PTR_ERR(oldfid);
>
> -       name = __getname();
> -       if (unlikely(!name)) {
> -               retval = -ENOMEM;
> -               goto clunk_fid;
> -       }
> -
>         sprintf(name, "%d\n", oldfid->fid);
>         retval = v9fs_vfs_mkspecial(dir, dentry, P9_DMLINK, name);
> -       __putname(name);
>         if (!retval) {
>                 v9fs_refresh_inode(oldfid, d_inode(old_dentry));
>                 v9fs_invalidate_inode_attr(dir);
>         }
> -clunk_fid:
>         p9_client_clunk(oldfid);
>         return retval;
>  }
> @@ -1425,7 +1419,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
> *dentry, umode_t mode, dev_t rde
>  {
>         struct v9fs_session_info *v9ses = v9fs_inode2v9ses(dir);
>         int retval;
> -       char *name;
> +       char name[2 + U32_MAX_DIGITS + 1 + U32_MAX_DIGITS + 1];
>         u32 perm;
>
>         p9_debug(P9_DEBUG_VFS, " %lu,%pd mode: %hx MAJOR: %u MINOR: %u\n",
> @@ -1435,26 +1429,16 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
> *dentry, umode_t mode, dev_t rde
>         if (!new_valid_dev(rdev))
>                 return -EINVAL;
>
> -       name = __getname();
> -       if (!name)
> -               return -ENOMEM;
>         /* build extension */
>         if (S_ISBLK(mode))
>                 sprintf(name, "b %u %u", MAJOR(rdev), MINOR(rdev));
>         else if (S_ISCHR(mode))
>                 sprintf(name, "c %u %u", MAJOR(rdev), MINOR(rdev));
> -       else if (S_ISFIFO(mode))
> -               *name = 0;
> -       else if (S_ISSOCK(mode))
> +       else
>                 *name = 0;
> -       else {
> -               __putname(name);
> -               return -EINVAL;
> -       }
>
>         perm = unixmode2p9mode(v9ses, mode);
>         retval = v9fs_vfs_mkspecial(dir, dentry, perm, name);
> -       __putname(name);
>
>         return retval;
>  }
>

[-- Attachment #2: Type: text/html, Size: 4927 bytes --]

Re: [Qemu-devel] [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Al Viro]

On Tue, Apr 14, 2015 at 04:19:41PM +0000, Eric Van Hensbergen wrote:
> That patch looks fine by me.  Happy to put it in the queue.  Thanks Al.

	OK...  Here's one more:

9p: don't bother with __getname() in ->follow_link()

We copy there a kmalloc'ed string and proceed to kfree that string immediately
after that.  Easier to just feed that string to nd_set_link() and _not_
kfree it until ->put_link() (which becomes kfree_put_link() in that case).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h
index 099c771..48d35d8 100644
--- a/fs/9p/v9fs.h
+++ b/fs/9p/v9fs.h
@@ -150,8 +150,6 @@ extern int v9fs_vfs_unlink(struct inode *i, struct dentry *d);
 extern int v9fs_vfs_rmdir(struct inode *i, struct dentry *d);
 extern int v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			struct inode *new_dir, struct dentry *new_dentry);
-extern void v9fs_vfs_put_link(struct dentry *dentry, struct nameidata *nd,
-			void *p);
 extern struct inode *v9fs_inode_from_fid(struct v9fs_session_info *v9ses,
 					 struct p9_fid *fid,
 					 struct super_block *sb, int new);
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index cda68f7..0ba1171 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1224,103 +1224,46 @@ ino_t v9fs_qid2ino(struct p9_qid *qid)
 }
 
 /**
- * v9fs_readlink - read a symlink's location (internal version)
+ * v9fs_vfs_follow_link - follow a symlink path
  * @dentry: dentry for symlink
- * @buffer: buffer to load symlink location into
- * @buflen: length of buffer
+ * @nd: nameidata
  *
  */
 
-static int v9fs_readlink(struct dentry *dentry, char *buffer, int buflen)
+static void *v9fs_vfs_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
-	int retval;
-
-	struct v9fs_session_info *v9ses;
-	struct p9_fid *fid;
+	struct v9fs_session_info *v9ses = v9fs_dentry2v9ses(dentry);
+	struct p9_fid *fid = v9fs_fid_lookup(dentry);
 	struct p9_wstat *st;
 
-	p9_debug(P9_DEBUG_VFS, " %pd\n", dentry);
-	retval = -EPERM;
-	v9ses = v9fs_dentry2v9ses(dentry);
-	fid = v9fs_fid_lookup(dentry);
+	p9_debug(P9_DEBUG_VFS, "%pd\n", dentry);
+
 	if (IS_ERR(fid))
-		return PTR_ERR(fid);
+		return ERR_CAST(fid);
 
 	if (!v9fs_proto_dotu(v9ses))
-		return -EBADF;
+		return ERR_PTR(-EBADF);
 
 	st = p9_client_stat(fid);
 	if (IS_ERR(st))
-		return PTR_ERR(st);
+		return ERR_CAST(st);
 
 	if (!(st->mode & P9_DMSYMLINK)) {
-		retval = -EINVAL;
-		goto done;
+		p9stat_free(st);
+		kfree(st);
+		return ERR_PTR(-EINVAL);
 	}
+	if (strlen(st->extension) >= PATH_MAX)
+		st->extension[PATH_MAX - 1] = '\0';
 
-	/* copy extension buffer into buffer */
-	retval = min(strlen(st->extension)+1, (size_t)buflen);
-	memcpy(buffer, st->extension, retval);
-
-	p9_debug(P9_DEBUG_VFS, "%pd -> %s (%.*s)\n",
-		 dentry, st->extension, buflen, buffer);
-
-done:
+	nd_set_link(nd, st->extension);
+	st->extension = NULL;
 	p9stat_free(st);
 	kfree(st);
-	return retval;
-}
-
-/**
- * v9fs_vfs_follow_link - follow a symlink path
- * @dentry: dentry for symlink
- * @nd: nameidata
- *
- */
-
-static void *v9fs_vfs_follow_link(struct dentry *dentry, struct nameidata *nd)
-{
-	int len = 0;
-	char *link = __getname();
-
-	p9_debug(P9_DEBUG_VFS, "%pd\n", dentry);
-
-	if (!link)
-		link = ERR_PTR(-ENOMEM);
-	else {
-		len = v9fs_readlink(dentry, link, PATH_MAX);
-
-		if (len < 0) {
-			__putname(link);
-			link = ERR_PTR(len);
-		} else
-			link[min(len, PATH_MAX-1)] = 0;
-	}
-	nd_set_link(nd, link);
-
 	return NULL;
 }
 
 /**
- * v9fs_vfs_put_link - release a symlink path
- * @dentry: dentry for symlink
- * @nd: nameidata
- * @p: unused
- *
- */
-
-void
-v9fs_vfs_put_link(struct dentry *dentry, struct nameidata *nd, void *p)
-{
-	char *s = nd_get_link(nd);
-
-	p9_debug(P9_DEBUG_VFS, " %pd %s\n",
-		 dentry, IS_ERR(s) ? "<error>" : s);
-	if (!IS_ERR(s))
-		__putname(s);
-}
-
-/**
  * v9fs_vfs_mkspecial - create a special file
  * @dir: inode to create special file in
  * @dentry: dentry to create
@@ -1514,7 +1457,7 @@ static const struct inode_operations v9fs_file_inode_operations = {
 static const struct inode_operations v9fs_symlink_inode_operations = {
 	.readlink = generic_readlink,
 	.follow_link = v9fs_vfs_follow_link,
-	.put_link = v9fs_vfs_put_link,
+	.put_link = kfree_put_link,
 	.getattr = v9fs_vfs_getattr,
 	.setattr = v9fs_vfs_setattr,
 };
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 9861c7c..bc2a91f 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -912,33 +912,18 @@ error:
 static void *
 v9fs_vfs_follow_link_dotl(struct dentry *dentry, struct nameidata *nd)
 {
-	int retval;
-	struct p9_fid *fid;
-	char *link = __getname();
+	struct p9_fid *fid = v9fs_fid_lookup(dentry);
 	char *target;
+	int retval;
 
 	p9_debug(P9_DEBUG_VFS, "%pd\n", dentry);
 
-	if (!link) {
-		link = ERR_PTR(-ENOMEM);
-		goto ndset;
-	}
-	fid = v9fs_fid_lookup(dentry);
-	if (IS_ERR(fid)) {
-		__putname(link);
-		link = ERR_CAST(fid);
-		goto ndset;
-	}
+	if (IS_ERR(fid))
+		return ERR_CAST(fid);
 	retval = p9_client_readlink(fid, &target);
-	if (!retval) {
-		strcpy(link, target);
-		kfree(target);
-		goto ndset;
-	}
-	__putname(link);
-	link = ERR_PTR(retval);
-ndset:
-	nd_set_link(nd, link);
+	if (retval)
+		return ERR_PTR(retval);
+	nd_set_link(nd, target);
 	return NULL;
 }
 
@@ -1006,7 +991,7 @@ const struct inode_operations v9fs_file_inode_operations_dotl = {
 const struct inode_operations v9fs_symlink_inode_operations_dotl = {
 	.readlink = generic_readlink,
 	.follow_link = v9fs_vfs_follow_link_dotl,
-	.put_link = v9fs_vfs_put_link,
+	.put_link = kfree_put_link,
 	.getattr = v9fs_vfs_getattr_dotl,
 	.setattr = v9fs_vfs_setattr_dotl,
 	.setxattr = generic_setxattr,

Re: [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Dominique Martinet]

Eric Van Hensbergen wrote on Mon, Apr 13, 2015 at 04:05:28PM +0000:
> Well, technically fid 3 isn't 'open', only fid 2 is open - at least
> according to the protocol.  fid 3 and fid 2 are both clones of fid 1.

Right, they're clone fids, but nothing in the protocol says what should
happen to non-open fids when you unlink the file either - I guess both
behaviours are OK as long as the client can handle it, so it would make
sense to at least call fstat() on the fid matching the fd, but while
I think this is how the kernel currently behaves the kernel doesn't HAVE
to make one open, separate fid per open file descriptor either.

> However, thanks for the alternative workaround.  If you get a chance, can
> you check that my change to the client to partially fix this for the other
> servers doesn't break nfs-ganesha:
> 
> https://github.com/ericvh/linux/commit/fddc7721d6d19e4e6be4905f37ade5b0521f4ed5

I'm afraid I haven't had much time lately, but while fstat-after-unlink
still works I'm getting some Oops with my basic test suite (create empty
files and rm -rf, kernel compilation, etc - nothing fancy) to the point
of locking myself out of my test box pretty quickly.

I'll try to debug patches a bit more individually (trying everything at
once isn't helping), but at the very least something is fishy

-- 
Dominique Martinet

Re: [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files

[Eric Van Hensbergen]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

good to know, thanks dominique.  I gave it a sniff test with FSX and a few
other benchmarks, but I need to hit it with some multithreaded
regressions.  Any pointers to reproducible failure cases would be
beneficial.

On Wed, Apr 15, 2015 at 6:28 AM Dominique Martinet <
dominique.martinet@cea.fr> wrote:

> Eric Van Hensbergen wrote on Mon, Apr 13, 2015 at 04:05:28PM +0000:
> > Well, technically fid 3 isn't 'open', only fid 2 is open - at least
> > according to the protocol.  fid 3 and fid 2 are both clones of fid 1.
>
> Right, they're clone fids, but nothing in the protocol says what should
> happen to non-open fids when you unlink the file either - I guess both
> behaviours are OK as long as the client can handle it, so it would make
> sense to at least call fstat() on the fid matching the fd, but while
> I think this is how the kernel currently behaves the kernel doesn't HAVE
> to make one open, separate fid per open file descriptor either.
>
> > However, thanks for the alternative workaround.  If you get a chance, can
> > you check that my change to the client to partially fix this for the
> other
> > servers doesn't break nfs-ganesha:
> >
> >
> https://github.com/ericvh/linux/commit/fddc7721d6d19e4e6be4905f37ade5b0521f4ed5
>
> I'm afraid I haven't had much time lately, but while fstat-after-unlink
> still works I'm getting some Oops with my basic test suite (create empty
> files and rm -rf, kernel compilation, etc - nothing fancy) to the point
> of locking myself out of my test box pretty quickly.
>
> I'll try to debug patches a bit more individually (trying everything at
> once isn't helping), but at the very least something is fishy
>
> --
> Dominique Martinet
>

[-- Attachment #2: Type: text/html, Size: 2184 bytes --]