Support single byte reads from integers published in procfs by kernel/sysctl.c

The following illustrates the problem:

    cat /proc/sys/kernel/threads-max | od -bc
    0000000 065 062 071 071 062 012
              5   2   9   9   2  \n
    0000006

    dd bs=1 < /proc/sys/kernel/threads-max | od -bc
    1+0 records in
    1+0 records out
    1 byte (1 B) copied, 0.000102707 s, 9.7 kB/s
    0000000 065
              5
    0000001

The implementation in sysctl.c is inconsistent because support for small reads is available for strings (eg /proc/sys/kernel/core_pattern), but not integers.

This issue was also reported in:

    https://bugzilla.kernel.org/show_bug.cgi?id=19182

Signed-off-by Earl Chew <[email protected]>

---
 kernel/sysctl.c |   83 +++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 57 insertions(+), 26 deletions(-)

kernel/sysctl.c +57 -26

index ae27196..b13d47b 100644
@@ -2215,31 +2215,41 @@ static int proc_get_long(char **buf, size_t *size,
  * @size: the size of the user buffer
  * @val: the integer to be converted
  * @neg: sign of the number, %TRUE for negative
+ * @skip: the number of characters to skip
  *
  * In case of success %0 is returned and @buf and @size are updated with
  * the amount of bytes written.
  */
 static int proc_put_long(void __user **buf, size_t *size, unsigned long val,
-			  bool neg)
+			  bool neg, loff_t *skip)
 {
 	int len;
 	char tmp[TMPBUFLEN], *p = tmp;
 
 	sprintf(p, "%s%lu", neg ? "-" : "", val);
-	len = strlen(tmp);
-	if (len > *size)
-		len = *size;
-	if (copy_to_user(*buf, tmp, len))
-		return -EFAULT;
-	*size -= len;
-	*buf += len;
+	len = strlen(p);
+	if (*skip >= len) {
+		*skip -= len;
+	} else {
+		p += *skip;
+		len -= *skip;
+		*skip = 0;
+		if (len > *size)
+			len = *size;
+		if (copy_to_user(*buf, p, len))
+			return -EFAULT;
+		*size -= len;
+		*buf += len;
+	}
 	return 0;
 }
 #undef TMPBUFLEN
 
-static int proc_put_char(void __user **buf, size_t *size, char c)
+static int proc_put_char(void __user **buf, size_t *size, char c, loff_t *skip)
 {
-	if (*size) {
+	if (*skip) {
+		(*skip)--;
+	} else if (*size) {
 		char __user **buffer = (char __user **)buf;
 		if (put_user(c, *buffer))
 			return -EFAULT;

@@ -2279,10 +2289,11 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 {
 	int *i, vleft, first = 1, err = 0;
 	unsigned long page = 0;
+	loff_t skip = *ppos;
 	size_t left;
 	char *kbuf;
 
-	if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
+	if (!tbl_data || !table->maxlen || !*lenp) {
 		*lenp = 0;
 		return 0;
 	}

@@ -2331,18 +2342,26 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 				err = -EINVAL;
 				break;
 			}
-			if (!first)
-				err = proc_put_char(&buffer, &left, '\t');
+			if (!first) {
+				err = proc_put_char(
+					&buffer, &left, '\t', &skip);
+			}
 			if (err)
 				break;
-			err = proc_put_long(&buffer, &left, lval, neg);
+			err = proc_put_long(&buffer, &left, lval, neg, &skip);
 			if (err)
 				break;
 		}
 	}
 
-	if (!write && !first && left && !err)
-		err = proc_put_char(&buffer, &left, '\n');
+	if (!write) {
+		if (!first && !err)
+			err = proc_put_char(&buffer, &left, '\n', &skip);
+		if (!err && skip) {
+			*lenp = 0;
+			return 0;
+		}
+	}
 	if (write && !err && left)
 		left -= proc_skip_spaces(&kbuf);
 free:

@@ -2497,10 +2516,11 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 	unsigned long *i, *min, *max;
 	int vleft, first = 1, err = 0;
 	unsigned long page = 0;
+	loff_t skip = *ppos;
 	size_t left;
 	char *kbuf;
 
-	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
+	if (!data || !table->maxlen || !*lenp) {
 		*lenp = 0;
 		return 0;
 	}

@@ -2546,15 +2566,22 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 		} else {
 			val = convdiv * (*i) / convmul;
 			if (!first)
-				err = proc_put_char(&buffer, &left, '\t');
-			err = proc_put_long(&buffer, &left, val, false);
+				err = proc_put_char(
+					&buffer, &left, '\t', &skip);
+			err = proc_put_long(&buffer, &left, val, false, &skip);
 			if (err)
 				break;
 		}
 	}
 
-	if (!write && !first && left && !err)
-		err = proc_put_char(&buffer, &left, '\n');
+	if (!write) {
+		if (!first && !err)
+			err = proc_put_char(&buffer, &left, '\n', &skip);
+		if (!err && skip) {
+			*lenp = 0;
+			return 0;
+		}
+	}
 	if (write && !err)
 		left -= proc_skip_spaces(&kbuf);
 free:

@@ -2805,6 +2832,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 	int err = 0;
 	bool first = 1;
 	size_t left = *lenp;
+	loff_t skip = *ppos;
 	unsigned long bitmap_len = table->maxlen;
 	unsigned long *bitmap = (unsigned long *) table->data;
 	unsigned long *tmp_bitmap = NULL;

@@ -2893,18 +2921,21 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 						   bit_a + 1) - 1;
 
 			if (!first) {
-				err = proc_put_char(&buffer, &left, ',');
+				err = proc_put_char(&buffer, &left, ',', &skip);
 				if (err)
 					break;
 			}
-			err = proc_put_long(&buffer, &left, bit_a, false);
+			err = proc_put_long(
+				&buffer, &left, bit_a, false, &skip);
 			if (err)
 				break;
 			if (bit_a != bit_b) {
-				err = proc_put_char(&buffer, &left, '-');
+				err = proc_put_char(
+					&buffer, &left, '-', &skip);
 				if (err)
 					break;
-				err = proc_put_long(&buffer, &left, bit_b, false);
+				err = proc_put_long(
+					&buffer, &left, bit_b, false, &skip);
 				if (err)
 					break;
 			}

@@ -2912,7 +2943,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 			first = 0; bit_b++;
 		}
 		if (!err)
-			err = proc_put_char(&buffer, &left, '\n');
+			err = proc_put_char(&buffer, &left, '\n', &skip);
 	}
 
 	if (!err) {

The difference between strings and integers is that with strings single byte reads/writes actually make sense.

Single decimal digits reads don't make much sense at all for integers.

Is there any compelling reason to support this complete idiocy from user space programs?

Quoted 7 lines · Eric W. Biederman

The difference between strings and integers is that with strings single byte reads/writes actually make sense.

Single decimal digits reads don't make much sense at all for integers.

Is there any compelling reason to support this complete idiocy from user space programs?

Eric,

[ I also note that the patch allows multiple short reads (not necessarily
just a single byte at a time) from these entries in procfs. ]

I think there is a reasonable userspace expectation that entities that present themselves as text files to produce results that are consistent with the userspace model of a text file.

For a more concrete example, consider the following BusyBox ash(1) code:

read X < /proc/sys/kernel/threads-max
Y=$(cat /proc/sys/kernel/threads-max)

read A < /proc/sys/kernel/core_pattern
B=$(cat /proc/sys/kernel/core_pattern)

The fact that these yield different results is surprising given that the user sees these as text files.

I think it is worthwhile asking the opposite question,

" Is there any compelling reason for the kernel to require userspace
programs to access /proc/sys/kernel/threads-max any differently from, say, /etc/fstab ? "

If the answer is in the affirmative, then it requires some layer in the userspace program (presumably the IO layer) to make some special accommodation for some entries in /proc.

As can be seen in the case of BusyBox, this approach might be difficult to swallow in something as generic as ash(1), so it is left to the user to puzzle out why read fails, but cat works -- but only sometimes.

Quoted 13 lines

[ I also note that the patch allows multiple short reads (not necessarily
just a single byte at a time) from these entries in procfs. ]

I think there is a reasonable userspace expectation that entities that present themselves as text files to produce results that are consistent with the userspace model of a text file.

For a more concrete example, consider the following BusyBox ash(1) code:

read X < /proc/sys/kernel/threads-max
Y=$(cat /proc/sys/kernel/threads-max)

Quoted 6 lines

read A < /proc/sys/kernel/core_pattern
B=$(cat /proc/sys/kernel/core_pattern)

The fact that these yield different results is surprising given that the user sees these as text files.

read X < file seems like a reasonable case to support. Bash doesn't have that problem so presumably BusyBox is simply inefficient.

If you are interested in fixing this properly with a tiny buffer reachable from struct file I think this can be worth fixing. I think this is doable by using seq_file in proc_sys_read.

Rereading different bytes of the integer multiple times when the integer may be changing does not seem like a reasonable implementation.

Quoted 2 lines

Rereading different bytes of the integer multiple times when the integer may be changing does not seem like a reasonable implementation.

Yes. I agree with you. I shall re-work the patch as per your suggestion.

Quoted 3 lines · On Mon, 23 Jan 2012 08:47:51 -0800 Earl Chew <[email protected]> wrote:

Quoted 2 lines

Rereading different bytes of the integer multiple times when the integer may be changing does not seem like a reasonable implementation.

Quoted 1 line

Yes. I agree with you. I shall re-work the patch as per your suggestion.

Yes, this is a bug and procfs should support byte-at-a-time reading of these strings. And yes, they are strings! Of digits.

We fixed an instance of this in procfs a while ago (maybe a year ago?). But I forget where it was. It is surprising that a bug of this nature survived so long.

We fixed an instance of this in procfs a while ago (maybe a year ago?). But I forget where it was. It is surprising that a bug of this nature survived so long.

If you are interested in fixing this properly with a tiny buffer reachable from struct file I think this can be worth fixing. I think this is doable by using seq_file in proc_sys_read.

Quoted 3 lines

If you are interested in fixing this properly with a tiny buffer reachable from struct file I think this can be worth fixing. I think this is doable by using seq_file in proc_sys_read.

I've looked into making proc_sys_read() use seq_file, but there are a few issues to work through.

I'm assuming that the existing kernel modules must continue to use the ctl_table/proc_dointvec/etc interface without requiring patching.

The two main consequences of this are:

a. The existing struct ctl_table interface should be preserved.
Kernel modules use this to publish into procfs.

b. The existing proc_dointvec(), etc, functions must be preserved
because they are exported via EXPORT_SYMBOL. Kernel modules may rely on these symbols in arbitrary ways.

I tried use seq_file in proc_sys_read, and it comes close to solving the problem. The issue is that proc_dointvec() requires a __user pointer.

A seq_file has an internal buffer that could be filled by proc_dointvec() -- but that seq_file buffer is in kernel space.

This is not a problem isolated to the seq_file implementation. I think that any approach involving a small local buffer would mean that that buffer is in kernel space, and that buffer cannot be passed to proc_dointvec() as it stands now because proc_dointvec() requires a __user buffer.

One approach that might work is to add a new field to struct ctl_table :

struct ctl_table

{
	...
	proc_seq_handler *proc_seq_handler;
};

Existing modules can continue to use proc_dointvec() etc and fixed code can use the new proc_seq_handler interface using the new proc_seq_dointvec() etc.

Although this preserves the old interface, it seems to me that this approach is flawed in that kernel modules using struct ctl_table and proc_dointvec() continue to be broken -- they are not fixed.

So, perhaps breaking assumption (b) might be a reasonable thing to do ?

If we accept that proc_dointvec() etc are only or mainly used in the context of filling out struct ctl_table, and that other kernel modules don't use proc_dointvec() in other contexts, then we can change the call signature of proc_dointvec() to stop using a __user pointer:

1. Change signature of proc_dointvec() etc to stop using __user pointer for reads
2. Change definition of typedef proc_call_handler to stop using __user pointer for reads
3. In kernel/sysctl.c don't use copy_to_user() for reads

Then proc_sys_read() can use proc_dointvec() etc to fill seq_file.

For writes, the existing behaviour needs to be preserved. One approach that would solve this would be to add:

	union proc_buffer {
		void __user *uptr;
		void *kptr;
	};

	typedef int proc_handler (struct ctl_table *ctl, int write,
                          union proc_buffer buffer, size_t *lenp, loff_t *ppos);

I think this would be preferable to either casting away __user for reads, or adding two pointers to the proc_handler signature (one with __user, one without).

If write is indicated, use buffer.uptr, and if not the handlers would use buffer.kptr.

But this is not perfect. Kernel modules containing their _own_ proc_handler definitions would now be broken severely.

And now I circle back to the proc_seq_handler approach. Each ctl_table client must be fixed individually (ie switch from proc_handler to proc_seq_handler).

What do you think ?

I suspect what we want for writes is to read the existing value into an internal buffer, and then allow partial writes to the internal buffer.

I don't know if writable seq_files exist.

/*
* seq_files support lseek() and pread(). They do not implement
* write() at all, but we clear FMODE_PWRITE here for historical
* reasons.
*
* If a client of seq_files a) implements file.write() and b) wishes to
* support pwrite() then that client will need to implement its own
* file.open() which calls seq_open() and then sets FMODE_PWRITE. */
file->f_mode &= ~FMODE_PWRITE;