Re: compat_linux: open: Supporting O_NOATIME

[Aiman Najjar <aiman.najjar%hurranet.com@localhost>]

Hi tech-kern@,

I've prepared a patch for this below, BUT, in retrospect, I am not sure how 
consequential this fix is. The privilege check exists only to prevent 
processes from bypassing file access time "atime" update if they don't 
own the file or are not root, but we don't plan to offer that behavior 
anyway, i.e. atime will always be updated regardless of whether a 
process passes or fails the checks.

In other words, the patch is mostly cosmetic for purposes of passing the 
LTP test. With that in mind, I came up with these available 
options based on my research:

1. Do nothing, Linux binaries that pass O_NOATIME will still work. 
Unless a program uses this API to validate ownership of files or root 
privileges (unlikely and weird), program correctness should not be 
affected.

2. Implement a naive check. My patch does that, it checks file ownership 
when O_NOATIME is set, then delegates to the native sys_open(at) 
implementation. This suffers from a potential TOCTOU, but keeps the 
compat_linux layer a thin shim. It has performance implications as it 
will resolve the path twice, which would be even worse for backup software 
than doing nothing.

3. Implement a more robust check: Based on my research, it seems a 
MODULE_HOOK is the right way to add compatibility code inside the 
native sys_open(at) path. This would solve issues in #2 but would turn 
compat_linux into more than a translation layer, which does not seem to 
be worth it in this case.

4. Maybe there are other options that don't suffer from issues in #2 and 
still get us to pass the LTP test, would be happy to look into those 
based on any feedback.

In closing, I did not actually submit a patch to GNATS as my personal 
assessment is that option #1 (doing nothing) is probably better, but 
would appreciate your comments otherwise, regardless, it has been a very 
useful learning exercise, I appreciated the well-written kernel 
developer manual pages :)


diff --git a/sys/compat/linux/common/linux_file.c b/sys/compat/linux/common/linux_file.c
index 66b95278e..42a79491a 100644
--- a/sys/compat/linux/common/linux_file.c
+++ b/sys/compat/linux/common/linux_file.c
@@ -233,10 +233,38 @@ linux_sys_open(struct lwp *l, const struct linux_sys_open_args *uap,
 		syscallarg(int) flags;
 		syscallarg(linux_umode_t) mode;
 	} */
-	int error, fl;
+	int error, fl, lflags, uid;
 	struct sys_open_args boa;
+	struct pathbuf *pb;
+	struct nameidata nd;
+	struct vattr va;
 
 	fl = linux_to_bsd_ioflags(SCARG(uap, flags));
+	lflags = SCARG(uap, flags);
+	uid = kauth_cred_geteuid(l->l_cred);
+
+	if (lflags & LINUX_O_NOATIME) {
+		error = pathbuf_copyin(SCARG(uap, path), &pb);
+		if (error)
+			return error;
+
+		NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, pb);
+		if ((error = namei(&nd)) == 0) {
+			if ((error = VOP_GETATTR(nd.ni_vp, &va, l->l_cred)))
+				goto out;
+
+			if (uid != va.va_uid &&
+			    kauth_authorize_generic(l->l_cred,
+				KAUTH_GENERIC_ISSUSER, NULL) != 0) {
+				error = EPERM;
+			}
+		out:
+			vput(nd.ni_vp);
+		}
+		pathbuf_destroy(pb);
+		if (error)
+			return error;
+	}
 
 	memset(&boa, 0, sizeof(boa));
 	SCARG(&boa, path) = SCARG(uap, path);
@@ -260,10 +288,55 @@ linux_sys_openat(struct lwp *l, const struct linux_sys_openat_args *uap,
 		syscallarg(int) flags;
 		syscallarg(linux_umode_t) mode;
 	} */
-	int error, fl;
+	int error, fl, lflags, uid;
 	struct sys_openat_args boa;
+	struct pathbuf *pb;
+	struct nameidata nd;
+	struct vattr va;
+	struct vnode *dvp = NULL;
+	const char *pathstring = NULL;
+	file_t *dfp = NULL;
 
 	fl = linux_to_bsd_ioflags(SCARG(uap, flags));
+	lflags = SCARG(uap, flags);
+	uid = kauth_cred_geteuid(l->l_cred);
+
+	if (lflags & LINUX_O_NOATIME) {
+		error = pathbuf_copyin(SCARG(uap, path), &pb);
+		if (error)
+			return error;
+
+		pathstring = pathbuf_stringcopy_get(pb);
+		if (SCARG(uap, fd) != AT_FDCWD && pathstring[0] != '/') {
+			if ((error = fd_getvnode(SCARG(uap, fd), &dfp)) != 0)
+				goto out2;
+			dvp = dfp->f_vnode;
+		}
+
+		NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, pb);
+		if (dvp != NULL)
+			NDAT(&nd, dvp);
+
+		if ((error = namei(&nd)) == 0) {
+			if ((error = VOP_GETATTR(nd.ni_vp, &va, l->l_cred)))
+				goto out1;
+
+			if (uid != va.va_uid &&
+			    kauth_authorize_generic(l->l_cred,
+				KAUTH_GENERIC_ISSUSER, NULL) != 0) {
+				error = EPERM;
+			}
+out1:
+			vput(nd.ni_vp);
+		}
+		if (dvp != NULL)
+			fd_putfile(SCARG(uap, fd));
+out2:
+		pathbuf_stringcopy_put(pb, pathstring);
+		pathbuf_destroy(pb);
+		if (error)
+			return error;
+	}
 
 	memset(&boa, 0, sizeof(boa));
 	SCARG(&boa, fd) = SCARG(uap, fd);