Fix proc_file_write missing ppos update

The following fix a long standing issue in the proc_file_write function, which doesn't update the ppos file position pointer.

This prevent the usage of multiple sequently writes on an opened proc file, because it is impossible to distinguish these due the offset is always 0.

Signed-off-by Stefani Seibold <[email protected]>

 generic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/proc/generic.c +0 -0

@@ -219,9 +219,10 @@
 		pde->pde_users++;

Quoted 16 lines · On Fri, 07 Aug 2009 22:27:10 +0200 Stefani Seibold <[email protected]> wrote:

The following fix a long standing issue in the proc_file_write function, which doesn't update the ppos file position pointer.

This prevent the usage of multiple sequently writes on an opened proc file, because it is impossible to distinguish these due the offset is always 0.

Signed-off-by Stefani Seibold <[email protected]>

 generic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/proc/generic.c +0 -0

@@ -219,9 +219,10 @@
 		pde->pde_users++;

fs/proc/generic.c line 228

Quoted 10 lines

fs/proc/generic.c +2 -1

 		spin_unlock(&pde->pde_unload_lock);
 
-		/* FIXME: does this routine need ppos?  probably... */
 		rv = pde->write_proc(file, buffer, count, pde->data);
 		pde_users_dec(pde);
+		if (rv > 0)
+			*ppos += rv;
 	}
 	return rv;
 }

Yes, that's odd.

I worry that there might be procfs write handlers which are looking at *ppos and whose behaviour might be altered by this patch.

<searches a bit>

Look at arch/s390/appldata/appldata_base.c:appldata_timer_handler().

static int
appldata_timer_handler(ctl_table *ctl, int write, struct file *filp,
void __user *buffer, size_t *lenp, loff_t *ppos)

{
	int len;
	char buf[2];

	if (!*lenp || *ppos) {
		*lenp = 0;
		return 0;
	}

Prior to your change, an application which opened that proc file and repeatedly wrote to the fd would repeatedly start and stop the timer.

After your change, the second and successive writes would have no effect unless the application was changed to lseek back to the start of the "file".

And that was just the second file I looked at via

	$EDITOR $(grep -l '[*]ppos' $(grep -rl _proc_ .))

Quoted 17 lines

fs/proc/generic.c line 228

Quoted 4 lines

fs/proc/generic.c +1 -0

+			*ppos += rv;
 	}
 	return rv;
 }

Yes, that's odd.

I worry that there might be procfs write handlers which are looking at *ppos and whose behaviour might be altered by this patch.

<searches a bit>

Look at arch/s390/appldata/appldata_base.c:appldata_timer_handler().

static int
appldata_timer_handler(ctl_table *ctl, int write, struct file *filp,
void __user *buffer, size_t *lenp, loff_t *ppos)

Quoted 10 lines

{
	int len;
	char buf[2];

	if (!*lenp || *ppos) {
		*lenp = 0;
		return 0;
	}

This function will be handled IMHO by the proc_sys_call_handler which has nothing to do with the proc_file_write.
/proc/sys/... file access should be not touched because there are handled differently.

Quoted 7 lines

Prior to your change, an application which opened that proc file and repeatedly wrote to the fd would repeatedly start and stop the timer.

After your change, the second and successive writes would have no effect unless the application was changed to lseek back to the start of the "file".

Quoted 3 lines

And that was just the second file I looked at via

	$EDITOR $(grep -l '[*]ppos' $(grep -rl _proc_ .))

Yes, i think you are right, i have forseen also that there maybe some pitfalls. The question is: is there any appplication which will be broken by this patch?

So what is your suggestion? Should we drop this patch or should we analyze the users and fix it?

My opinion is to fix it, because it is wrong and it limits the usage of the proc_write operation. Many embedded developers like me count on proc support, because it is much simpler to use than the seqfile thing.

Quoted 2 lines · On Fri, 07 Aug 2009 23:43:07 +0200 Stefani Seibold <[email protected]> wrote:

So what is your suggestion? Should we drop this patch or should we analyze the users and fix it?

Well.

We could review all implementations of ->write_proc. There only seem to be twenty or so.

If any of them will have their behaviour altered by this patch then let's look at those on a case-by-case basis and decide whether making this change will have an acceptable risk.

If we _do_ find one for which we simply cannot make this behavioural change then.. ugh. We could perhaps add a new `bool proc_dir_entry.implement_old_broken_behaviour' and set that flag for the offending driver(s) and test it within proc_write_file().

Or we could do

	if (pde->write_proc_new) {
		rv = pde->write_proc_new(file, buffer, count, pde->data);
		*ppos += rv;
	} else {
		rv = pde->write_proc(file, buffer, count, pde->data);
	}

which is really the same thing and isn't obviously better ;)

Quoted 3 lines

My opinion is to fix it, because it is wrong and it limits the usage of the proc_write operation. Many embedded developers like me count on proc support, because it is much simpler to use than the seqfile thing.

Quoted 30 lines · Andrew Morton

Quoted 2 lines · On Fri, 07 Aug 2009 23:43:07 +0200 Stefani Seibold <[email protected]> wrote:

So what is your suggestion? Should we drop this patch or should we analyze the users and fix it?

Well.

We could review all implementations of ->write_proc. There only seem to be twenty or so.

If any of them will have their behaviour altered by this patch then let's look at those on a case-by-case basis and decide whether making this change will have an acceptable risk.

If we _do_ find one for which we simply cannot make this behavioural change then.. ugh. We could perhaps add a new `bool proc_dir_entry.implement_old_broken_behaviour' and set that flag for the offending driver(s) and test it within proc_write_file().

Or we could do

	if (pde->write_proc_new) {
		rv = pde->write_proc_new(file, buffer, count, pde->data);
		*ppos += rv;
	} else {
		rv = pde->write_proc(file, buffer, count, pde->data);
	}

Quoted 5 lines

which is really the same thing and isn't obviously better ;)

Quoted 3 lines

My opinion is to fix it, because it is wrong and it limits the usage of the proc_write operation. Many embedded developers like me count on proc support, because it is much simpler to use than the seqfile thing.

The simple and portable answer is to implement your own file_operations.

It is unlikely that implementing a new totally unstructured proc file is a good idea.

I'm not quite up to speed on write_proc but I believe we have been spraying read_proc and write_proc because of problems with the interface.