On Tue, Apr 21, 2020 at 08:16:15PM +0100, Al Viro wrote:

On Tue, Apr 21, 2020 at 07:15:39PM +0200, Christoph Hellwig wrote:

quoted

Instead of having all the sysctl handlers deal with user pointers, which
is rather hairy in terms of the BPF interaction, copy the input to and
from  userspace in common code.  This also means that the strings are
always NUL-terminated by the common code, making the API a little bit
safer.

As most handler just pass through the data to one of the common handlers
a lot of the changes are mechnical.

quoted

@@ -564,27 +564,38 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *buf,
 	if (!table->proc_handler)
 		goto out;
 
-	error = BPF_CGROUP_RUN_PROG_SYSCTL(head, table, write, buf, &count,
-					   ppos, &new_buf);
+	if (write) {
+		kbuf = memdup_user_nul(ubuf, count);
+		if (IS_ERR(kbuf)) {
+			error = PTR_ERR(kbuf);
+			goto out;
+		}
+	} else {
+		error = -ENOMEM;
+		kbuf = kzalloc(count, GFP_KERNEL);

Better allocate count + 1 bytes here, that way a lot of insanity in the
instances can be simply converted to snprintf().  Yes, I know it'll bring
the Church Of Avoiding The Abomination Of Sprintf out of the woodwork,
but...

FWIW, consider e.g. net/sunrpc/sysctl.c:

Nevermind that the read side should be simply
		int err = proc_douintvec(table, write, buffer, lenp, ppos);
		/* Display the RPC tasks on writing to rpc_debug */
		if (!err && strcmp(table->procname, "rpc_debug") == 0)
			rpc_show_tasks(&init_net);
		return err;
the write side would become
		len = snprintf(buffer, *lenp + 1, "0x%04x\n",
				*(unsigned int *)table->data);
		if (len > *lenp)
			len = *lenp;
		*lenp -= len;
		*ppos += len;
		return 0;
and I really wonder if lifting the trailing boilerplate into the caller would've
been better.  Note that e.g. gems like
                        if (!first)
                                err = proc_put_char(&buffer, &left, '\t');
                        if (err)
                                break;
                        err = proc_put_long(&buffer, &left, lval, neg);
                        if (err)
                                break;
are due to lack of snprintf-to-user; now, lose the "to user" part and we suddenly
can be rid of that stuff...