[PATCH 1/2] Move chdir/chroot-related fdp manipulation to kern_descrip.c

Mateusz Guzik mjguzik at gmail.com
Sat Jul 11 13:40:30 UTC 2015


On Sat, Jul 11, 2015 at 03:43:35PM +0300, Konstantin Belousov wrote:
> On Sat, Jul 11, 2015 at 01:08:03AM +0200, Mateusz Guzik wrote:
> > From: Mateusz Guzik <mjg at freebsd.org>
> > 
> > Prefix exported functions with pwd_.
> > 
> > This also adds a helper which sets fd_cdir which deduplicates some code.
> Patch is fine, I like the cleanup. Please use the opportunity to fix
> minor existing issues, mostly with style, see below.
> 
> > +	oldvp = fdp->fd_rdir;
> VREF(vp);
> > +	fdp->fd_rdir = vp;
> > +	VREF(fdp->fd_rdir);
> Remove this line.
> 
> > +	if (!fdp->fd_jdir) {
> if (fdp->fd_jdir == NULL) {
> 
> > +		fdp->fd_jdir = vp;
> > +		VREF(fdp->fd_jdir);
> Again, swap the order: do VREF(vp), then assign.
> 

Done.
> > +	}
> > +	FILEDESC_XUNLOCK(fdp);
> > +	vrele(oldvp);
> > +	return (0);
> > +}
> > +
> > +void
> > +pwd_chdir(struct thread *td, struct vnode *vp)
> > +{
> > +	struct filedesc *fdp;
> > +	struct vnode *oldvp;
> > +
> > + 	fdp = td->td_proc->p_fd;
> > +	FILEDESC_XLOCK(fdp);
> > +	oldvp = fdp->fd_cdir;
> > +	fdp->fd_cdir = vp;
> Assert that vp v_usecount > 0, as the inaccurate but still useful check.
> We have no way to ensure that the reference is owned by the code, but
> we do know that there must be such reference.
> 

Done.

diff --git a/sys/compat/svr4/svr4_misc.c b/sys/compat/svr4/svr4_misc.c
index ec4504e..5e53874 100644
--- a/sys/compat/svr4/svr4_misc.c
+++ b/sys/compat/svr4/svr4_misc.c
@@ -643,7 +643,7 @@ svr4_sys_fchroot(td, uap)
 		goto fail;
 #endif
 	VOP_UNLOCK(vp, 0);
-	error = change_root(vp, td);
+	error = pwd_chroot(td, vp);
 	vrele(vp);
 	return (error);
 fail:
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index e8f0014..51e8b3c 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2875,6 +2875,96 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
 }
 
 /*
+ * This sysctl determines if we will allow a process to chroot(2) if it
+ * has a directory open:
+ *	0: disallowed for all processes.
+ *	1: allowed for processes that were not already chroot(2)'ed.
+ *	2: allowed for all processes.
+ */
+
+static int chroot_allow_open_directories = 1;
+
+SYSCTL_INT(_kern, OID_AUTO, chroot_allow_open_directories, CTLFLAG_RW,
+    &chroot_allow_open_directories, 0,
+    "Allow a process to chroot(2) if it has a directory open");
+
+/*
+ * Helper function for raised chroot(2) security function:  Refuse if
+ * any filedescriptors are open directories.
+ */
+static int
+chroot_refuse_vdir_fds(struct filedesc *fdp)
+{
+	struct vnode *vp;
+	struct file *fp;
+	int fd;
+
+	FILEDESC_LOCK_ASSERT(fdp);
+
+	for (fd = 0; fd <= fdp->fd_lastfile; fd++) {
+		fp = fget_locked(fdp, fd);
+		if (fp == NULL)
+			continue;
+		if (fp->f_type == DTYPE_VNODE) {
+			vp = fp->f_vnode;
+			if (vp->v_type == VDIR)
+				return (EPERM);
+		}
+	}
+	return (0);
+}
+
+/*
+ * Common routine for kern_chroot() and jail_attach().  The caller is
+ * responsible for invoking priv_check() and mac_vnode_check_chroot() to
+ * authorize this operation.
+ */
+int
+pwd_chroot(struct thread *td, struct vnode *vp)
+{
+	struct filedesc *fdp;
+	struct vnode *oldvp;
+	int error;
+
+	fdp = td->td_proc->p_fd;
+	FILEDESC_XLOCK(fdp);
+	if (chroot_allow_open_directories == 0 ||
+	    (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) {
+		error = chroot_refuse_vdir_fds(fdp);
+		if (error != 0) {
+			FILEDESC_XUNLOCK(fdp);
+			return (error);
+		}
+	}
+	oldvp = fdp->fd_rdir;
+	VREF(vp);
+	fdp->fd_rdir = vp;
+	if (fdp->fd_jdir == NULL) {
+		VREF(vp);
+		fdp->fd_jdir = vp;
+	}
+	FILEDESC_XUNLOCK(fdp);
+	vrele(oldvp);
+	return (0);
+}
+
+void
+pwd_chdir(struct thread *td, struct vnode *vp)
+{
+	struct filedesc *fdp;
+	struct vnode *oldvp;
+
+	fdp = td->td_proc->p_fd;
+	FILEDESC_XLOCK(fdp);
+	VNASSERT(vp->v_usecount > 0, vp,
+	    ("chdir to a vnode with zero usecount"));
+	oldvp = fdp->fd_cdir;
+	fdp->fd_cdir = vp;
+	FILEDESC_XUNLOCK(fdp);
+	vrele(oldvp);
+}
+
+/*
  * Scan all active processes and prisons to see if any of them have a current
  * or root directory of `olddp'. If so, replace them with the new mount point.
  */
diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c
index c118d74..6bdddd7 100644
--- a/sys/kern/kern_jail.c
+++ b/sys/kern/kern_jail.c
@@ -2432,7 +2432,7 @@ do_jail_attach(struct thread *td, struct prison *pr)
 		goto e_unlock;
 #endif
 	VOP_UNLOCK(pr->pr_root, 0);
-	if ((error = change_root(pr->pr_root, td)))
+	if ((error = pwd_chroot(td, pr->pr_root)))
 		goto e_revert_osd;
 
 	newcred = crget();
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 9088017..258a1ef 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -728,8 +728,7 @@ sys_fchdir(td, uap)
 		int fd;
 	} */ *uap;
 {
-	register struct filedesc *fdp = td->td_proc->p_fd;
-	struct vnode *vp, *tdp, *vpold;
+	struct vnode *vp, *tdp;
 	struct mount *mp;
 	struct file *fp;
 	cap_rights_t rights;
@@ -761,11 +760,7 @@ sys_fchdir(td, uap)
 		return (error);
 	}
 	VOP_UNLOCK(vp, 0);
-	FILEDESC_XLOCK(fdp);
-	vpold = fdp->fd_cdir;
-	fdp->fd_cdir = vp;
-	FILEDESC_XUNLOCK(fdp);
-	vrele(vpold);
+	pwd_chdir(td, vp);
 	return (0);
 }
 
@@ -791,9 +786,7 @@ sys_chdir(td, uap)
 int
 kern_chdir(struct thread *td, char *path, enum uio_seg pathseg)
 {
-	register struct filedesc *fdp = td->td_proc->p_fd;
 	struct nameidata nd;
-	struct vnode *vp;
 	int error;
 
 	NDINIT(&nd, LOOKUP, FOLLOW | LOCKSHARED | LOCKLEAF | AUDITVNODE1,
@@ -807,56 +800,11 @@ kern_chdir(struct thread *td, char *path, enum uio_seg pathseg)
 	}
 	VOP_UNLOCK(nd.ni_vp, 0);
 	NDFREE(&nd, NDF_ONLY_PNBUF);
-	FILEDESC_XLOCK(fdp);
-	vp = fdp->fd_cdir;
-	fdp->fd_cdir = nd.ni_vp;
-	FILEDESC_XUNLOCK(fdp);
-	vrele(vp);
-	return (0);
-}
-
-/*
- * Helper function for raised chroot(2) security function:  Refuse if
- * any filedescriptors are open directories.
- */
-static int
-chroot_refuse_vdir_fds(fdp)
-	struct filedesc *fdp;
-{
-	struct vnode *vp;
-	struct file *fp;
-	int fd;
-
-	FILEDESC_LOCK_ASSERT(fdp);
-
-	for (fd = 0; fd <= fdp->fd_lastfile; fd++) {
-		fp = fget_locked(fdp, fd);
-		if (fp == NULL)
-			continue;
-		if (fp->f_type == DTYPE_VNODE) {
-			vp = fp->f_vnode;
-			if (vp->v_type == VDIR)
-				return (EPERM);
-		}
-	}
+	pwd_chdir(td, nd.ni_vp);
 	return (0);
 }
 
 /*
- * This sysctl determines if we will allow a process to chroot(2) if it
- * has a directory open:
- *	0: disallowed for all processes.
- *	1: allowed for processes that were not already chroot(2)'ed.
- *	2: allowed for all processes.
- */
-
-static int chroot_allow_open_directories = 1;
-
-SYSCTL_INT(_kern, OID_AUTO, chroot_allow_open_directories, CTLFLAG_RW,
-     &chroot_allow_open_directories, 0,
-     "Allow a process to chroot(2) if it has a directory open");
-
-/*
  * Change notion of root (``/'') directory.
  */
 #ifndef _SYS_SYSPROTO_H_
@@ -891,7 +839,7 @@ sys_chroot(td, uap)
 		goto e_vunlock;
 #endif
 	VOP_UNLOCK(nd.ni_vp, 0);
-	error = change_root(nd.ni_vp, td);
+	error = pwd_chroot(td, nd.ni_vp);
 	vrele(nd.ni_vp);
 	NDFREE(&nd, NDF_ONLY_PNBUF);
 	return (error);
@@ -926,42 +874,6 @@ change_dir(vp, td)
 	return (VOP_ACCESS(vp, VEXEC, td->td_ucred, td));
 }
 
-/*
- * Common routine for kern_chroot() and jail_attach().  The caller is
- * responsible for invoking priv_check() and mac_vnode_check_chroot() to
- * authorize this operation.
- */
-int
-change_root(vp, td)
-	struct vnode *vp;
-	struct thread *td;
-{
-	struct filedesc *fdp;
-	struct vnode *oldvp;
-	int error;
-
-	fdp = td->td_proc->p_fd;
-	FILEDESC_XLOCK(fdp);
-	if (chroot_allow_open_directories == 0 ||
-	    (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) {
-		error = chroot_refuse_vdir_fds(fdp);
-		if (error != 0) {
-			FILEDESC_XUNLOCK(fdp);
-			return (error);
-		}
-	}
-	oldvp = fdp->fd_rdir;
-	fdp->fd_rdir = vp;
-	VREF(fdp->fd_rdir);
-	if (!fdp->fd_jdir) {
-		fdp->fd_jdir = vp;
-		VREF(fdp->fd_jdir);
-	}
-	FILEDESC_XUNLOCK(fdp);
-	vrele(oldvp);
-	return (0);
-}
-
 static __inline void
 flags_to_rights(int flags, cap_rights_t *rightsp)
 {
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index ab7ce9f..e569a3b 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -205,6 +205,10 @@ fd_modified(struct filedesc *fdp, int fd, seq_t seq)
 	return (!seq_consistent(fd_seq(fdp->fd_files, fd), seq));
 }
 
+/* cdir/rdir/jdir manipulation functions. */
+void	pwd_chdir(struct thread *td, struct vnode *vp);
+int	pwd_chroot(struct thread *td, struct vnode *vp);
+
 #endif /* _KERNEL */
 
 #endif /* !_SYS_FILEDESC_H_ */
diff --git a/sys/sys/vnode.h b/sys/sys/vnode.h
index 36ef8af..6d5da32 100644
--- a/sys/sys/vnode.h
+++ b/sys/sys/vnode.h
@@ -616,7 +616,6 @@ void	cache_purge(struct vnode *vp);
 void	cache_purge_negative(struct vnode *vp);
 void	cache_purgevfs(struct mount *mp);
 int	change_dir(struct vnode *vp, struct thread *td);
-int	change_root(struct vnode *vp, struct thread *td);
 void	cvtstat(struct stat *st, struct ostat *ost);
 void	cvtnstat(struct stat *sb, struct nstat *nsb);
 int	getnewvnode(const char *tag, struct mount *mp, struct vop_vector *vops,
diff --git a/sys/ufs/ffs/ffs_alloc.c b/sys/ufs/ffs/ffs_alloc.c
index 2b9c334..c587dfb 100644
--- a/sys/ufs/ffs/ffs_alloc.c
+++ b/sys/ufs/ffs/ffs_alloc.c
@@ -2748,13 +2748,12 @@ sysctl_ffs_fsck(SYSCTL_HANDLER_ARGS)
 	struct thread *td = curthread;
 	struct fsck_cmd cmd;
 	struct ufsmount *ump;
-	struct vnode *vp, *vpold, *dvp, *fdvp;
+	struct vnode *vp, *dvp, *fdvp;
 	struct inode *ip, *dp;
 	struct mount *mp;
 	struct fs *fs;
 	ufs2_daddr_t blkno;
 	long blkcnt, blksize;
-	struct filedesc *fdp;
 	struct file *fp, *vfp;
 	cap_rights_t rights;
 	int filetype, error;
@@ -2968,12 +2967,7 @@ sysctl_ffs_fsck(SYSCTL_HANDLER_ARGS)
 			break;
 		}
 		VOP_UNLOCK(vp, 0);
-		fdp = td->td_proc->p_fd;
-		FILEDESC_XLOCK(fdp);
-		vpold = fdp->fd_cdir;
-		fdp->fd_cdir = vp;
-		FILEDESC_XUNLOCK(fdp);
-		vrele(vpold);
+		pwd_chdir(td, vp);
 		break;
 
 	case FFS_SET_DOTDOT:
-- 
Mateusz Guzik <mjguzik gmail.com>


More information about the freebsd-fs mailing list