Fixing exec-modifies-atime

[Ken Smith]

Someone recently noted we don't set a file's access time when it
gets executed, and that's a violation of standards.

It turns out this had been discussed back in 2003 but nothing got
done at the time because there were several proposed solutions and
each of the solutions had their own set of tradeoffs involved.

I made an overly simplistic attempt at fixing this last week and
Bruce Evans was quick to point out the list of reasons that fix
was incorrect so it got backed out.  We have since discussed what
had been talked about back in 2003.  IMHO the patch tacked on below
is a good first step.  It fixes the problem in UFS and taking care
of the other filesystems that support access time should just be a
matter of adding something similar to their setattr function.

Does anyone see any problems with this approach, or any major flaws
with the implementation in this patch?

Thanks.

Index: sys/sys/vnode.h
===================================================================
RCS file: /home/ncvs/src/sys/sys/vnode.h,v
retrieving revision 1.276
diff -u -r1.276 vnode.h
--- sys/sys/vnode.h	28 Jan 2005 13:08:21 -0000	1.276
+++ sys/sys/vnode.h	2 Feb 2005 19:51:44 -0000
@@ -262,6 +262,7 @@
  */
 #define	VA_UTIMES_NULL	0x01		/* utimes argument was NULL */
 #define	VA_EXCLUSIVE	0x02		/* exclusive create request */
+#define	VA_EXECVE_ATIME	0x04		/* setting atime for execve */
 
 /*
  * Flags for ioflag. (high 16 bits used to ask for read-ahead and
Index: sys/kern/kern_exec.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_exec.c,v
retrieving revision 1.265
diff -u -r1.265 kern_exec.c
--- sys/kern/kern_exec.c	29 Jan 2005 23:51:05 -0000	1.265
+++ sys/kern/kern_exec.c	6 Feb 2005 22:59:54 -0000
@@ -278,10 +278,11 @@
 	struct ucred *newcred = NULL, *oldcred;
 	struct uidinfo *euip;
 	register_t *stack_base;
-	int error, len, i;
+	int error, len, i, start_write_err = 1;
 	struct image_params image_params, *imgp;
-	struct vattr attr;
+	struct vattr atimeattr, attr;
 	int (*img_first)(struct image_params *);
+	struct mount *mp;
 	struct pargs *oldargs = NULL, *newargs = NULL;
 	struct sigacts *oldsigacts, *newsigacts;
 #ifdef KTRACE
@@ -341,19 +342,27 @@
 
 	/*
 	 * Translate the file name. namei() returns a vnode pointer
-	 *	in ni_vp amoung other things.
+	 * in ni_vp among other things.  We don't lock it right away
+	 * because after obtaining the vnode we need to call vn_start_write()
+	 * and having the vnode locked while calling that is a bad idea.
 	 */
 	ndp = &nd;
-	NDINIT(ndp, LOOKUP, LOCKLEAF | FOLLOW | SAVENAME,
-	    UIO_SYSSPACE, args->fname, td);
-
+	NDINIT(ndp, LOOKUP, FOLLOW | SAVENAME, UIO_SYSSPACE, args->fname, td);
 	mtx_lock(&Giant);
 interpret:
-
 	error = namei(ndp);
 	if (error)
 		goto exec_fail;
 
+	/*
+	 * Notify the filesystem of an impending write.  We will update
+	 * the file's access time.  Lock the vnode afterwards.
+	 * vn_start_write() may block if a filesystem snapshot is in
+	 * progress.
+	 */
+	start_write_err = vn_start_write(ndp->ni_vp, &mp, V_WAIT | PCATCH);
+	vn_lock(ndp->ni_vp, LK_EXCLUSIVE | LK_RETRY, td);
+
 	imgp->vp = ndp->ni_vp;
 
 	/*
@@ -432,10 +441,12 @@
 		mac_copy_vnode_label(ndp->ni_vp->v_label, interplabel);
 #endif
 		vput(ndp->ni_vp);
+		if (!start_write_err)
+			vn_finished_write(mp);
 		vm_object_deallocate(imgp->object);
 		imgp->object = NULL;
 		/* set new name to that of the interpreter */
-		NDINIT(ndp, LOOKUP, LOCKLEAF | FOLLOW | SAVENAME,
+		NDINIT(ndp, LOOKUP, FOLLOW | SAVENAME,
 		    UIO_SYSSPACE, imgp->interpreter_name, td);
 		goto interpret;
 	}
@@ -672,6 +683,17 @@
 		exec_setregs(td, imgp->entry_addr,
 		    (u_long)(uintptr_t)stack_base, imgp->ps_strings);
 
+	/*
+	 * Here we should update the access time of the file.
+	 */
+	if (!(ndp->ni_vp->v_mount->mnt_flag & (MNT_NOATIME | MNT_RDONLY)) &&
+	    !start_write_err) {
+		VATTR_NULL(&atimeattr);
+		vfs_timestamp(&atimeattr.va_atime);
+		atimeattr.va_vaflags |= VA_EXECVE_ATIME;
+		(void) VOP_SETATTR(ndp->ni_vp, &atimeattr, td->td_ucred, td);
+	}
+
 done1:
 	/*
 	 * Free any resources malloc'd earlier that we didn't use.
@@ -739,6 +761,8 @@
 		if (interplabel != NULL)
 			mac_vnode_label_free(interplabel);
 #endif
+		if (!start_write_err)
+			vn_finished_write(mp);
 		mtx_unlock(&Giant);
 		exit1(td, W_EXITCODE(0, SIGABRT));
 		/* NOT REACHED */
@@ -750,6 +774,8 @@
 	if (interplabel != NULL)
 		mac_vnode_label_free(interplabel);
 #endif
+	if (!start_write_err)
+		vn_finished_write(mp);
 	mtx_unlock(&Giant);
 	return (error);
 }
Index: sys/ufs/ufs/ufs_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
retrieving revision 1.262
diff -u -r1.262 ufs_vnops.c
--- sys/ufs/ufs/ufs_vnops.c	2 Feb 2005 14:21:01 -0000	1.262
+++ sys/ufs/ufs/ufs_vnops.c	5 Feb 2005 18:51:11 -0000
@@ -507,6 +507,20 @@
 		if (vap->va_flags & (IMMUTABLE | APPEND))
 			return (0);
 	}
+	/*
+	 * Update the file access time when it has been executed.  We are
+	 * doing this here to specifically avoid some of the checks done
+	 * below - this operation is done by request of the kernel and
+	 * should bypass some security checks.  Things like read-only
+	 * checks do get handled by lower levels (e.g. ffs_update()).
+	 */
+	if (vap->va_vaflags & VA_EXECVE_ATIME) {
+		ip->i_flag &= ~IN_ACCESS;
+		ip->i_flag |= IN_LAZYMOD;
+		DIP_SET(ip, i_atime, vap->va_atime.tv_sec);
+		DIP_SET(ip, i_atimensec, vap->va_atime.tv_nsec);
+		return (0);
+	}
 	if (ip->i_flags & (IMMUTABLE | APPEND))
 		return (EPERM);
 	/*


-- 
						Ken Smith
- From there to here, from here to      |       kensmith at cse.buffalo.edu
  there, funny things are everywhere.   |
                      - Theodore Geisel |