Re: [PATCH] Fix proc_file_write missing ppos update

Re: [PATCH] Fix proc_file_write missing ppos update

[Andrew Morton]

On Fri, 07 Aug 2009 22:27:10 +0200
Stefani Seibold <stefani@seibold.net> 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 <stefani@seibold.net>
> 
>  generic.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.31-rc4.orig/fs/proc/generic.c	2009-08-07 22:05:57.000000000 +0200
> +++ linux-2.6.30-rc4/fs/proc/generic.c	2009-08-07 22:06:22.000000000 +0200
> @@ -219,9 +219,10 @@
>  		pde->pde_users++;
>  		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_ .))

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Freitag, den 07.08.2009, 13:58 -0700 schrieb Andrew Morton: 
> On Fri, 07 Aug 2009 22:27:10 +0200
> Stefani Seibold <stefani@seibold.net> 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 <stefani@seibold.net>
> > 
> >  generic.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > --- linux-2.6.31-rc4.orig/fs/proc/generic.c	2009-08-07 22:05:57.000000000 +0200
> > +++ linux-2.6.30-rc4/fs/proc/generic.c	2009-08-07 22:06:22.000000000 +0200
> > @@ -219,9 +219,10 @@
> >  		pde->pde_users++;
> >  		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;
> 	}
> 
> 

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. 

> 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_ .))

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.

Re: [PATCH] Fix proc_file_write missing ppos update

[Andrew Morton]

On Fri, 07 Aug 2009 23:43:07 +0200
Stefani Seibold <stefani@seibold.net> wrote:

> > > +			*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;
> > 	}
> > 
> > 
> 
> 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. 

hm, OK, fail.

> > 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_ .))
> 
> 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?

There is no way of telling.  We have to assume that there will be such
code out there.

> 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 ;)

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

Re: [PATCH] Fix proc_file_write missing ppos update

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Fri, 07 Aug 2009 23:43:07 +0200
> Stefani Seibold <stefani@seibold.net> 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 ;)
>
>> 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.

Eric

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Freitag, den 07.08.2009, 23:59 -0700 schrieb Eric W. Biederman:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Fri, 07 Aug 2009 23:43:07 +0200
> > Stefani Seibold <stefani@seibold.net> 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 ;)
> >
> >> 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.
> 

This is what i still doing since a long time:

<CodeSnip>
 proc_entry = create_proc_entry(procname, S_IRUGO|S_IWUGO, NULL);

 proc_entry->read_proc = proc_read_foo;

 bar->proc_file_operations.llseek = proc_entry->proc_fops->llseek;
 bar->proc_file_operations.read = proc_entry->proc_fops->read;
 bar->proc_file_operations.write = proc_write_foo;

 proc_entry->proc_fops = &bar->proc_file_operations;
</CodeSnip>

This works very well for me, but it requires some additional step
because of the buggy interface.

But the question is: can we fix this bug? 

I will have a look on the current users of proc->write and if there are
no driver which is depending on the old behavior we can fix it. 
   
> It is unlikely that implementing a new totally unstructured proc file is
> a good idea.
> 

That is your opinion. I still use it f.e. to access a eeprom.
 
> 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.
> 

First: I never noticed a problem with the current proc interface. The
only issue i figured out is the proc_write ppos problem.

Second: If speed matters or not is a question of the use case. Sometimes
a simple solution is required.  

Stefani