Re: Ptyfs correction.

[Ilia Zykov <netbsd%izyk.ru@localhost>]

On 13.10.2014 19:12, Christos Zoulas wrote:
> In article <543BBE4A.7090608%izyk.ru@localhost>, Ilia Zykov  <netbsd%izyk.ru@localhost> wrote:
>> -=-=-=-=-=-
>>
>> Sorry, forgot free().
> 
> I think I would prefer to has a lock argument in the lookup function
> and lock for the first call (the plain lookup) and not lock in the
> second one (the lookup to insert). This feels safer to me and I don't
> have to explain all the intricacies why locking is not needed which
> just makes the code more fragile.
> 
> christos
> 

Hello!
 - corrects some wrong comments
 - add XXX warning
 - increase security
 - make pty_vn_open() private to tty_ptm.c

Thank you.
Ilia.

 fs/ptyfs/ptyfs_subr.c   |    6 ++++++
 fs/ptyfs/ptyfs_vfsops.c |    6 ++++--
 kern/tty_ptm.c          |   17 +----------------
 sys/pty.h               |    3 +--
 4 files changed, 12 insertions(+), 20 deletions(-)

Index: fs/ptyfs/ptyfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/fs/ptyfs/ptyfs_subr.c,v
retrieving revision 1.29.4.1
diff -u -p -r1.29.4.1 ptyfs_subr.c
--- fs/ptyfs/ptyfs_subr.c	17 Aug 2014 03:34:02 -0000	1.29.4.1
+++ fs/ptyfs/ptyfs_subr.c	15 Oct 2014 09:05:22 -0000
@@ -177,6 +177,12 @@ ptyfs_get_node(ptyfstype type, int pty)
 	    pp->ptyfs_atime = pp->ptyfs_ctime;
 	pp->ptyfs_flags = 0;
 	mutex_enter(&ptyfs_hashlock);
+	/*
+	 * XXX We have minimum race condition when opening master side
+	 * first time, if other threads through other mount points, trying
+	 * opening the same device. As follow we have little chance have
+	 * unused list entries.
+	 */
 	SLIST_INSERT_HEAD(ppp, pp, ptyfs_hash);
 	mutex_exit(&ptyfs_hashlock);
 	return pp;
Index: fs/ptyfs/ptyfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/ptyfs/ptyfs_vfsops.c,v
retrieving revision 1.50.2.1
diff -u -p -r1.50.2.1 ptyfs_vfsops.c
--- fs/ptyfs/ptyfs_vfsops.c	17 Aug 2014 03:34:02 -0000	1.50.2.1
+++ fs/ptyfs/ptyfs_vfsops.c	15 Oct 2014 09:05:22 -0000
@@ -217,7 +217,8 @@ ptyfs__allocvp(struct mount *mp, struct 
 		*vpp = NULL;
 		return error;
 	}
-	if (type == PTYFSptc)
+	/* Activate node only after we have grabbed device. */
+	if (type == PTYFSpts)
 		ptyfs_set_active(mp, minor(dev));
 	return 0;
 }
@@ -415,7 +416,8 @@ ptyfs_sync(struct mount *mp, int waitfor
 
 /*
  * Initialize this vnode / ptynode pair.
- * Caller assures no other thread will try to load this node.
+ * Only for the slave side of a pty, caller assures
+ * no other thread will try to load this node.
  */
 int
 ptyfs_loadvnode(struct mount *mp, struct vnode *vp,
Index: kern/tty_ptm.c
===================================================================
RCS file: /cvsroot/src/sys/kern/tty_ptm.c,v
retrieving revision 1.33
diff -u -p -r1.33 tty_ptm.c
--- kern/tty_ptm.c	25 Jul 2014 08:10:40 -0000	1.33
+++ kern/tty_ptm.c	15 Oct 2014 09:05:22 -0000
@@ -87,6 +87,7 @@ int pts_major, ptc_major;
 static dev_t pty_getfree(void);
 static int pty_alloc_master(struct lwp *, int *, dev_t *, struct mount *);
 static int pty_alloc_slave(struct lwp *, int *, dev_t, struct mount *);
+static int pty_vn_open(struct vnode *, struct lwp *);
 
 void ptmattach(int);
 
@@ -174,22 +175,6 @@ retry:
 		error = EOPNOTSUPP;
 		goto bad;
 	}
-	/*
-	 * XXX Since PTYFS has now multiple instance support, if we mounted
-	 * more than one PTYFS we must check here the ptyfs_used_tbl, to find
-	 * out if the ptyfsnode is under the appropriate mount and skip the
-	 * node if not, because the pty could has been released, but
-	 * ptyfs_reclaim didn't get a chance to release the corresponding
-	 * node other mount point yet.
-	 *
-	 * It's important to have only one mount point's ptyfsnode for each
-	 * appropriate device in ptyfs_used_tbl, else we will have a security 
-	 * problem, because every entry will have access to this device.
-	 *
-	 * Also we will not have not efficient vnode and memory usage.
-	 * You can test this by changing a_recycle from true to false
-	 * in ptyfs_inactive.
-	 */
 	if ((error = (*ptm->allocvp)(mp, l, &vp, *dev, 'p')) != 0) {
 		DPRINTF(("pty_allocvp %d\n", error));
 		goto bad;
Index: sys/pty.h
===================================================================
RCS file: /cvsroot/src/sys/sys/pty.h,v
retrieving revision 1.10
diff -u -p -r1.10 pty.h
--- sys/pty.h	4 Apr 2014 18:11:58 -0000	1.10
+++ sys/pty.h	15 Oct 2014 09:05:23 -0000
@@ -39,14 +39,13 @@ void ptmattach(int);
 int pty_fill_ptmget(struct lwp *, dev_t, int, int, void *, struct mount *);
 int pty_grant_slave(struct lwp *, dev_t, struct mount *);
 dev_t pty_makedev(char, int);
-int pty_vn_open(struct vnode *, struct lwp *);
 struct ptm_pty *pty_sethandler(struct ptm_pty *);
 int pty_getmp(struct lwp *, struct mount **);
 
 /*
  * Ptm_pty is used for switch ptm{x} driver between BSDPTY, PTYFS.
  * Functions' argument (struct mount *) is used only PTYFS,
- * in the case BSDPTY can be NULL, and arg must be NULL.
+ * in the case BSDPTY can be NULL.
  */
 struct ptm_pty {
 	int (*allocvp)(struct mount *, struct lwp *, struct vnode **, dev_t,