Re: [PATCH] Support single byte reads from integers published in procfs by kernel/sysctl.c

[Earl Chew <echew@ixiacom.com>]

> 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 ?


Earl