From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Van Hensbergen Subject: Re: [V9fs-developer] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files Date: Tue, 14 Apr 2015 16:19:41 +0000 Message-ID: References: <20140702135258.23882.15100.malonedeb@soybean.canonical.com> <20150410123059.26540.1036.malone@gac.canonical.com> <20150413082753.GA15891@u-blusson> <20150414160737.GX889@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a113cd8761fcf780513b19949 Cc: Linux FS Devel , Dominique Martinet , V9FS Developers , qemu-devel , Bug 1336794 <1336794@bugs.launchpad.net> To: Al Viro Return-path: In-Reply-To: <20150414160737.GX889@ZenIV.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: linux-fsdevel.vger.kernel.org --001a113cd8761fcf780513b19949 Content-Type: text/plain; charset=UTF-8 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 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; > } > --001a113cd8761fcf780513b19949 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
That patch looks fine by me.=C2=A0 Happy to put it in the = queue.=C2=A0 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.=C2=A0 fid 3 and fid 2 are both clones of fi= d 1.
> However, thanks for the alternative workaround.=C2=A0 If you get a cha= nce, can
> you check that my change to the client to partially fix this for the o= ther
> servers doesn't break nfs-ganesha:
>
> https://github.com/ericvh/linux/com= mit/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.=C2=A0 What's w= rong with
a local array of char?=C2=A0 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 instea= d
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 *de= ntry, const char *symname)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return v9fs_vfs_mkspecial(dir, dentry, P9_DMSYM= LINK, symname);
=C2=A0}

+#define U32_MAX_DIGITS 10
+
=C2=A0/**
=C2=A0 * v9fs_vfs_link - create a hardlink
=C2=A0 * @old_dentry: dentry for file to link to
@@ -1383,7 +1385,7 @@ v9fs_vfs_link(struct dentry *old_dentry, struct inode= *dir,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct dentry *dentry)
=C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 int retval;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0char *name;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0char name[1 + U32_MAX_DIGITS + 2]; /* sign + nu= mber + \n + \0 */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct p9_fid *oldfid;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 p9_debug(P9_DEBUG_VFS, " %lu,%pd,%pd\n&quo= t;,
@@ -1393,20 +1395,12 @@ v9fs_vfs_link(struct dentry *old_dentry, struct ino= de *dir,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(oldfid))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return PTR_ERR(oldf= id);

-=C2=A0 =C2=A0 =C2=A0 =C2=A0name =3D __getname();
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (unlikely(!name)) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0retval =3D -ENOMEM;=
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto clunk_fid;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0}
-
=C2=A0 =C2=A0 =C2=A0 =C2=A0 sprintf(name, "%d\n", oldfid->fid)= ;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 retval =3D v9fs_vfs_mkspecial(dir, dentry, P9_D= MLINK, name);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0__putname(name);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!retval) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 v9fs_refresh_inode(= oldfid, d_inode(old_dentry));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 v9fs_invalidate_ino= de_attr(dir);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
-clunk_fid:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 p9_client_clunk(oldfid);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return retval;
=C2=A0}
@@ -1425,7 +1419,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry *dent= ry, umode_t mode, dev_t rde
=C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct v9fs_session_info *v9ses =3D v9fs_inode2= v9ses(dir);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 int retval;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0char *name;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0char name[2 + U32_MAX_DIGITS + 1 + U32_MAX_DIGI= TS + 1];
=C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 perm;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 *de= ntry, umode_t mode, dev_t rde
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!new_valid_dev(rdev))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL;

-=C2=A0 =C2=A0 =C2=A0 =C2=A0name =3D __getname();
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!name)
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* build extension */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (S_ISBLK(mode))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sprintf(name, "= ;b %u %u", MAJOR(rdev), MINOR(rdev));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 else if (S_ISCHR(mode))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sprintf(name, "= ;c %u %u", MAJOR(rdev), MINOR(rdev));
-=C2=A0 =C2=A0 =C2=A0 =C2=A0else if (S_ISFIFO(mode))
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*name =3D 0;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0else if (S_ISSOCK(mode))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0else
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *name =3D 0;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0else {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__putname(name); -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0 =C2=A0 perm =3D unixmode2p9mode(v9ses, mode);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 retval =3D v9fs_vfs_mkspecial(dir, dentry, perm= , name);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0__putname(name);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 return retval;
=C2=A0}
--001a113cd8761fcf780513b19949--