Re: [PATCH] add setutsname to reuse code in setdomainname and sethostname

[Bradley Morgan <brads@mainlining.org>]

> +static int setutsname(char __user *name, int len, enum uts_proc uts_proc,
> +		      char (*field)[__NEW_UTS_LEN + 1],
> +		      int fieldlen)

If we pass uts_proc, why do we also need field? Nothing keeps them in sync.
UTS_PROC_HOSTNAME paired with &u->domainname still writes where you wanted,
it just notifies the wrong sysctl poll, and the compiler says nothing about
it. Both callsites are right today so its not a bug, its a shape I would
rather not have in the tree. The enum already names the two writable
fields,
so let the helper pick the field from it. fieldlen can go too, the array in
the parameter list already says 65, so sizeof(*field) is the whole field. I
built that bit standalone to be sure, it gives 65.

Diff on top of your patch, it applies clean and builds here:

diff --git a/kernel/sys.c b/kernel/sys.c
index 3a4e483..f957871 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1419,9 +1419,7 @@ SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
 }
 #endif
 
-static int setutsname(char __user *name, int len, enum uts_proc uts_proc,
-		      char (*field)[__NEW_UTS_LEN + 1],
-		      int fieldlen)
+static int setutsname(char __user *name, int len, enum uts_proc proc)
 {
 	int errno;
 	char tmp[__NEW_UTS_LEN];
@@ -1433,13 +1431,17 @@ static int setutsname(char __user *name, int len, enum uts_proc uts_proc,
 		return -EINVAL;
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
+		struct new_utsname *u;
+		char *field;
+
 		add_device_randomness(tmp, len);
 		down_write(&uts_sem);
-		memcpy(*field, tmp, len);
-		memset(*field + len, 0, fieldlen - len);
-
+		u = utsname();
+		field = proc == UTS_PROC_HOSTNAME ? u->nodename : u->domainname;
+		memcpy(field, tmp, len);
+		memset(field + len, 0, sizeof_field(struct new_utsname, nodename) - len);
 		errno = 0;
-		uts_proc_notify(uts_proc);
+		uts_proc_notify(proc);
 		up_write(&uts_sem);
 	}
 	return errno;
@@ -1447,10 +1449,7 @@ static int setutsname(char __user *name, int len, enum uts_proc uts_proc,
 
 SYSCALL_DEFINE2(sethostname, char __user *, name, int, len)
 {
-	// utsname() always returns a valid pointer to the current utsname structure.
-	struct new_utsname *u = utsname();
-
-	return setutsname(name, len, UTS_PROC_HOSTNAME, &u->nodename, sizeof(u->nodename));
+	return setutsname(name, len, UTS_PROC_HOSTNAME);
 }
 
 #ifdef __ARCH_WANT_SYS_GETHOSTNAME
@@ -1483,10 +1482,7 @@ SYSCALL_DEFINE2(gethostname, char __user *, name, int, len)
  */
 SYSCALL_DEFINE2(setdomainname, char __user *, name, int, len)
 {
-	// utsname() always returns a valid pointer to the current utsname structure.
-	struct new_utsname *u = utsname();
-
-	return setutsname(name, len, UTS_PROC_DOMAINNAME, &u->domainname, sizeof(u->domainname));
+	return setutsname(name, len, UTS_PROC_DOMAINNAME);
 }
 
 /* make sure you are allowed to change @tsk limits before calling this */

Then both syscalls are a one line return, 12 added lines against your 18.
If
you would rather the other procs be loud instead of silently taken as
domainname, make it a switch with WARN_ON_ONCE(1) in the default, I'd take
either form.


> +	// utsname() always returns a valid pointer to the current utsname structure.
> +	struct new_utsname *u = utsname();

I dont think reading utsname() outside the lock is unsafe, current owns its
nsproxy and nothing else can move uts_ns under it. But the hoist buys
nothing and it is a change your changelog does not mention: uts_sem used to
cover the call. Keeping it next to the memcpy means there is nothing to
explain. The comment restates the line it sits on and it appears twice, so
the diff drops it. Also kernel/sys.c has no // comments of its own, the
single // in it is the SPDX line, so it reads new for this file either way.
Anything that isnt needed, goes.

If you want more of a cleanup, the /proc/sys/kernel/
hostname writers in kernel/utsname_sysctl.c buffer, call
add_device_randomness(), take uts_sem and memcpy, so they are a third copy
of this. Not the same though, they write all 65 bytes rather than len plus
the zero fill, so afaics your patch doesn't try to cover them and that is
fine. But that file has had "Should this be in kernel/sys.c ????" sitting
over proc_do_uts_string() since 2007, so if you want the bigger version of
this cleanup, that is where the shared helper goes.
--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/