On Apr 2, 10:36am, net...@izyk.ru (Ilya Zykov) wrote: -- Subject: Re: Enhance ptyfs to handle multiple instances.
Looks very good. Some changes: - I don't like the refactoring because it makes ptyfs less optional (brings in code and headers to the base kernel). I think it is simpler to provide an entry function to get the mount point instead, and this way all the guts of ptyfs stay in ptyfs. - Is it important to append to the list? Then perhaps use a different set of macros than LIST_. I've changed the code just to prepend. I hope I did not break it. Comments? christos Index: sys/pty.h =================================================================== RCS file: /cvsroot/src/sys/sys/pty.h,v retrieving revision 1.9 diff -u -p -u -r1.9 pty.h --- sys/pty.h 27 Mar 2014 17:31:56 -0000 1.9 +++ sys/pty.h 3 Apr 2014 21:51:03 -0000 @@ -41,7 +41,7 @@ int pty_grant_slave(struct lwp *, dev_t, dev_t pty_makedev(char, int); int pty_vn_open(struct vnode *, struct lwp *); struct ptm_pty *pty_sethandler(struct ptm_pty *); -int ptyfs_getmp(struct lwp *, struct mount **); +int pty_getmp(struct lwp *, struct mount **); /* * Ptm_pty is used for switch ptm{x} driver between BSDPTY, PTYFS. @@ -53,7 +53,7 @@ struct ptm_pty { char); int (*makename)(struct mount *, struct lwp *, char *, size_t, dev_t, char); void (*getvattr)(struct mount *, struct lwp *, struct vattr *); - void *arg; + int (*getmp)(struct lwp *, struct mount **); }; #ifdef COMPAT_BSDPTY Index: fs/ptyfs/ptyfs.h =================================================================== RCS file: /cvsroot/src/sys/fs/ptyfs/ptyfs.h,v retrieving revision 1.11 diff -u -p -u -r1.11 ptyfs.h --- fs/ptyfs/ptyfs.h 21 Mar 2014 17:21:53 -0000 1.11 +++ fs/ptyfs/ptyfs.h 3 Apr 2014 21:51:04 -0000 @@ -106,6 +106,8 @@ struct ptyfsnode { }; struct ptyfsmount { + LIST_ENTRY(ptyfsmount) pmnt_le; + struct mount *pmnt_mp; gid_t pmnt_gid; mode_t pmnt_mode; int pmnt_flags; Index: fs/ptyfs/ptyfs_vfsops.c =================================================================== RCS file: /cvsroot/src/sys/fs/ptyfs/ptyfs_vfsops.c,v retrieving revision 1.48 diff -u -p -u -r1.48 ptyfs_vfsops.c --- fs/ptyfs/ptyfs_vfsops.c 27 Mar 2014 17:31:56 -0000 1.48 +++ fs/ptyfs/ptyfs_vfsops.c 3 Apr 2014 21:51:04 -0000 @@ -77,6 +77,7 @@ static int ptyfs__allocvp(struct mount * static int ptyfs__makename(struct mount *, struct lwp *, char *, size_t, dev_t, char); static void ptyfs__getvattr(struct mount *, struct lwp *, struct vattr *); +static int ptyfs__getmp(struct lwp *, struct mount **); /* * ptm glue: When we mount, we make ptm point to us. @@ -84,13 +85,37 @@ static void ptyfs__getvattr(struct mount struct ptm_pty *ptyfs_save_ptm; static int ptyfs_count; +static LIST_HEAD(, ptyfsmount) ptyfs_head; + struct ptm_pty ptm_ptyfspty = { ptyfs__allocvp, ptyfs__makename, ptyfs__getvattr, - NULL + ptyfs__getmp, }; +static int +ptyfs__getmp(struct lwp *l, struct mount **mpp) +{ + struct cwdinfo *cwdi = l->l_proc->p_cwdi; + struct mount *mp; + struct ptyfsmount *pmnt; + + LIST_FOREACH(pmnt, &ptyfs_head, pmnt_le) { + mp = pmnt->pmnt_mp; + if (cwdi->cwdi_rdir == NULL) + goto ok; + + if (vn_isunder(mp->mnt_vnodecovered, cwdi->cwdi_rdir, l)) + goto ok; + } + *mpp = NULL; + return EOPNOTSUPP; +ok: + *mpp = mp; + return 0; +} + static const char * ptyfs__getpath(struct lwp *l, const struct mount *mp) { @@ -137,6 +162,18 @@ ptyfs__makename(struct mount *mp, struct len = snprintf(tbuf, bufsiz, "/dev/null"); break; case 't': + /* + * We support traditional ptys, so we can get here, + * if pty had been opened before PTYFS was mounted, + * or was opened through /dev/ptyXX devices. + * Return it only outside chroot for more security :). + */ + if (l->l_proc->p_cwdi->cwdi_rdir == NULL + && ptyfs_save_ptm != NULL + && ptyfs_used_get(PTYFSptc, minor(dev), mp, 0) == NULL) + return (*ptyfs_save_ptm->makename)(mp, l, + tbuf, bufsiz, dev, ms); + np = ptyfs__getpath(l, mp); if (np == NULL) return EOPNOTSUPP; @@ -189,6 +226,7 @@ void ptyfs_init(void) { + LIST_INIT(&ptyfs_head); malloc_type_attach(M_PTYFSMNT); malloc_type_attach(M_PTYFSTMP); ptyfs_hashinit(); @@ -274,9 +312,9 @@ ptyfs_mount(struct mount *mp, const char return error; } - /* Point pty access to us */ - if (ptyfs_count == 0) { - ptm_ptyfspty.arg = mp; + LIST_INSERT_HEAD(&ptyfs_head, pmnt, pmnt_le); + if (ptyfs_count++ == 0) { + /* Point pty access to us */ ptyfs_save_ptm = pty_sethandler(&ptm_ptyfspty); } ptyfs_count++; @@ -296,6 +334,7 @@ ptyfs_unmount(struct mount *mp, int mntf { int error; int flags = 0; + struct ptyfsmount *pmnt; if (mntflags & MNT_FORCE) flags |= FORCECLOSE; @@ -308,8 +347,13 @@ ptyfs_unmount(struct mount *mp, int mntf /* Restore where pty access was pointing */ (void)pty_sethandler(ptyfs_save_ptm); ptyfs_save_ptm = NULL; - ptm_ptyfspty.arg = NULL; } + LIST_FOREACH(pmnt, &ptyfs_head, pmnt_le) { + if (pmnt->pmnt_mp == mp) { + LIST_REMOVE(pmnt, pmnt_le); + break; + } + } /* * Finally, throw away the ptyfsmount structure Index: fs/ptyfs/ptyfs_vnops.c =================================================================== RCS file: /cvsroot/src/sys/fs/ptyfs/ptyfs_vnops.c,v retrieving revision 1.45 diff -u -p -u -r1.45 ptyfs_vnops.c --- fs/ptyfs/ptyfs_vnops.c 27 Mar 2014 21:13:06 -0000 1.45 +++ fs/ptyfs/ptyfs_vnops.c 3 Apr 2014 21:51:04 -0000 @@ -141,6 +141,7 @@ int ptyfs_readdir (void *); #define ptyfs_readlink genfs_eopnotsupp #define ptyfs_abortop genfs_abortop int ptyfs_reclaim (void *); +int ptyfs_inactive (void *); #define ptyfs_lock genfs_lock #define ptyfs_unlock genfs_unlock #define ptyfs_bmap genfs_badop @@ -192,7 +193,7 @@ const struct vnodeopv_entry_desc ptyfs_v { &vop_readdir_desc, ptyfs_readdir }, /* readdir */ { &vop_readlink_desc, ptyfs_readlink }, /* readlink */ { &vop_abortop_desc, ptyfs_abortop }, /* abortop */ - { &vop_inactive_desc, spec_inactive }, /* inactive */ + { &vop_inactive_desc, ptyfs_inactive }, /* inactive */ { &vop_reclaim_desc, ptyfs_reclaim }, /* reclaim */ { &vop_lock_desc, ptyfs_lock }, /* lock */ { &vop_unlock_desc, ptyfs_unlock }, /* unlock */ @@ -225,6 +226,28 @@ ptyfs_reclaim(void *v) return ptyfs_freevp(ap->a_vp); } +int +ptyfs_inactive(void *v) +{ + struct vop_inactive_args /* { + struct vnode *a_vp; + bool *a_recycle; + } */ *ap = v; + struct vnode *vp = ap->a_vp; + struct ptyfsnode *ptyfs = VTOPTYFS(vp); + + switch (ptyfs->ptyfs_type) { + case PTYFSpts: + case PTYFSptc: + /* Emulate file deletion for call reclaim(). */ + *ap->a_recycle = true; + break; + default: + break; + } + return spec_inactive(v); +} + /* * Return POSIX pathconf information applicable to special devices. */ Index: kern/tty_bsdpty.c =================================================================== RCS file: /cvsroot/src/sys/kern/tty_bsdpty.c,v retrieving revision 1.19 diff -u -p -u -r1.19 tty_bsdpty.c --- kern/tty_bsdpty.c 27 Mar 2014 17:31:56 -0000 1.19 +++ kern/tty_bsdpty.c 3 Apr 2014 21:51:04 -0000 @@ -74,12 +74,13 @@ static int pty_makename(struct mount *, static int pty_allocvp(struct mount *, struct lwp *, struct vnode **, dev_t, char); static void pty_getvattr(struct mount *, struct lwp *, struct vattr *); +static intl pty_getmp(struct lwp *, struct mount **); struct ptm_pty ptm_bsdpty = { pty_allocvp, pty_makename, pty_getvattr, - NULL + pty_getmp, }; static int @@ -152,5 +153,13 @@ pty_getvattr(struct mount *mp, struct lw vattr->va_gid = TTY_GID; vattr->va_mode = TTY_PERM; } + +static int +pty_getmp(struct lwp *l __unused, struct mount **mpp) +{ + *mpp = 0; + return 0; +} + #endif /* COMPAT_BSDPTY */ #endif /* NO_DEV_PTM */ Index: kern/tty_ptm.c =================================================================== RCS file: /cvsroot/src/sys/kern/tty_ptm.c,v retrieving revision 1.31 diff -u -p -u -r1.31 tty_ptm.c --- kern/tty_ptm.c 27 Mar 2014 17:31:56 -0000 1.31 +++ kern/tty_ptm.c 3 Apr 2014 21:51:04 -0000 @@ -90,31 +90,12 @@ static int pty_alloc_slave(struct lwp *, void ptmattach(int); int -ptyfs_getmp(struct lwp *l, struct mount **mpp) { - struct cwdinfo *cwdi = l->l_proc->p_cwdi; - struct mount *mp; - +pty_getmp(struct lwp *l, struct mount **mpp) +{ if (ptm == NULL) return EOPNOTSUPP; - if (ptm->arg == NULL) { /* BSDPTY */ - *mpp = NULL; - return 0; - } - - mp = ptm->arg; /* PTYFS */ - - if (cwdi->cwdi_rdir == NULL) - goto ok; - - if (vn_isunder(mp->mnt_vnodecovered, cwdi->cwdi_rdir, l)) - goto ok; - - *mpp = NULL; - return EOPNOTSUPP; -ok: - *mpp = mp; - return 0; + return (*ptm->getmp)(l, mpp); } dev_t @@ -192,6 +173,22 @@ 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; @@ -355,7 +352,7 @@ ptmopen(dev_t dev, int flag, int mode, s switch(minor(dev)) { case 0: /* /dev/ptmx */ case 2: /* /emul/linux/dev/ptmx */ - if ((error = ptyfs_getmp(l, &mp)) != 0) + if ((error = pty_getmp(l, &mp)) != 0) return error; if ((error = pty_alloc_master(l, &fd, &ttydev, mp)) != 0) return error; @@ -403,7 +400,7 @@ ptmioctl(dev_t dev, u_long cmd, void *da error = 0; switch (cmd) { case TIOCPTMGET: - if ((error = ptyfs_getmp(l, &mp)) != 0) + if ((error = pty_getmp(l, &mp)) != 0) return error; if ((error = pty_alloc_master(l, &cfd, &newdev, mp)) != 0) Index: kern/tty_pty.c =================================================================== RCS file: /cvsroot/src/sys/kern/tty_pty.c,v retrieving revision 1.137 diff -u -p -u -r1.137 tty_pty.c --- kern/tty_pty.c 28 Mar 2014 11:55:09 -0000 1.137 +++ kern/tty_pty.c 3 Apr 2014 21:51:04 -0000 @@ -1075,7 +1075,7 @@ ptyioctl(dev_t dev, u_long cmd, void *da #ifndef NO_DEV_PTM /* Allow getting the name from either the master or the slave */ if (cmd == TIOCPTSNAME) { - if ((error = ptyfs_getmp(l, &mp)) != 0) + if ((error = pty_getmp(l, &mp)) != 0) return error; return pty_fill_ptmget(l, dev, -1, -1, data, mp); } @@ -1086,7 +1086,7 @@ ptyioctl(dev_t dev, u_long cmd, void *da switch (cmd) { #ifndef NO_DEV_PTM case TIOCGRANTPT: - if ((error = ptyfs_getmp(l, &mp)) != 0) + if ((error = pty_getmp(l, &mp)) != 0) return error; return pty_grant_slave(l, dev, mp); #endif