Re: patch "Staging: iio: adis16220: fix up some sysfs attribute permissions" added to staging tree

Re: patch "Staging: iio: adis16220: fix up some sysfs attribute permissions" added to staging tree

[Jonathan Cameron]

On 11/16/10 19:25, gregkh@suse.de wrote:
> 
> This is a note to let you know that I've just added the patch titled
> 
>     Staging: iio: adis16220: fix up some sysfs attribute permissions
> 
> to my staging git tree which can be found at
>     git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git
> in the staging-linus branch.
> 
> The patch will show up in the next release of the linux-next tree
> (usually sometime within the next 24 hours during the week.)
> 
> The patch will hopefully also will be merged in Linus's tree for the
> next -rc kernel release.
> 
> If you have any questions about this process, please let me know.
> 
> 
>>>From 1d904e8950c86e670ace237eaea1d48cd81e94df Mon Sep 17 00:00:00 2001
> From: Greg Kroah-Hartman <gregkh@suse.de>
> Date: Tue, 16 Nov 2010 11:19:53 -0800
> Subject: Staging: iio: adis16220: fix up some sysfs attribute permissions
> 
> They should not be writable by any user
Sorry Greg, it may be jet lag related, but I really don't see how this change
makes sense...  Doesn't this take a write only parameter and make it read only
with no read function defined?  Was the intent to restrict who could write
to this attribute?

> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Cc: Barry Song <Barry.Song@analog.com>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> ---
>  drivers/staging/iio/accel/adis16220_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c
> index c86d149..c2b2091 100644
> --- a/drivers/staging/iio/accel/adis16220_core.c
> +++ b/drivers/staging/iio/accel/adis16220_core.c
> @@ -507,7 +507,7 @@ static IIO_DEVICE_ATTR(reset, S_IWUSR, NULL,
>  		adis16220_write_reset, 0);
>  
>  #define IIO_DEV_ATTR_CAPTURE(_store)				\
> -	IIO_DEVICE_ATTR(capture, S_IWUGO, NULL, _store, 0)
> +	IIO_DEVICE_ATTR(capture, S_IRUSR, NULL, _store, 0)
>  
>  static IIO_DEV_ATTR_CAPTURE(adis16220_write_capture);
>  

Re: patch "Staging: iio: adis16220: fix up some sysfs attribute permissions" added to staging tree

[Greg KH]

On Thu, Nov 18, 2010 at 05:05:05PM +0000, Jonathan Cameron wrote:
> On 11/16/10 19:25, gregkh@suse.de wrote:
> > 
> > This is a note to let you know that I've just added the patch titled
> > 
> >     Staging: iio: adis16220: fix up some sysfs attribute permissions
> > 
> > to my staging git tree which can be found at
> >     git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git
> > in the staging-linus branch.
> > 
> > The patch will show up in the next release of the linux-next tree
> > (usually sometime within the next 24 hours during the week.)
> > 
> > The patch will hopefully also will be merged in Linus's tree for the
> > next -rc kernel release.
> > 
> > If you have any questions about this process, please let me know.
> > 
> > 
> >>From 1d904e8950c86e670ace237eaea1d48cd81e94df Mon Sep 17 00:00:00 2001
> > From: Greg Kroah-Hartman <gregkh@suse.de>
> > Date: Tue, 16 Nov 2010 11:19:53 -0800
> > Subject: Staging: iio: adis16220: fix up some sysfs attribute permissions
> > 
> > They should not be writable by any user
> Sorry Greg, it may be jet lag related, but I really don't see how this change
> makes sense...  Doesn't this take a write only parameter and make it read only
> with no read function defined?  Was the intent to restrict who could write
> to this attribute?

Ah crap.

> >  #define IIO_DEV_ATTR_CAPTURE(_store)				\
> > -	IIO_DEVICE_ATTR(capture, S_IWUGO, NULL, _store, 0)
> > +	IIO_DEVICE_ATTR(capture, S_IRUSR, NULL, _store, 0)

This should be S_IWUSR, right?

Did I just get all of these wrong?  I'll go verify them...

thanks,

greg k-h

Re: patch "Staging: iio: adis16220: fix up some sysfs attribute permissions" added to staging tree

[Jonathan Cameron]

On 11/18/10 17:19, Greg KH wrote:
> On Thu, Nov 18, 2010 at 05:05:05PM +0000, Jonathan Cameron wrote:
>> On 11/16/10 19:25, gregkh@suse.de wrote:
>>>
>>> This is a note to let you know that I've just added the patch titled
>>>
>>>     Staging: iio: adis16220: fix up some sysfs attribute permissions
>>>
>>> to my staging git tree which can be found at
>>>     git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git
>>> in the staging-linus branch.
>>>
>>> The patch will show up in the next release of the linux-next tree
>>> (usually sometime within the next 24 hours during the week.)
>>>
>>> The patch will hopefully also will be merged in Linus's tree for the
>>> next -rc kernel release.
>>>
>>> If you have any questions about this process, please let me know.
>>>
>>>
>>> >From 1d904e8950c86e670ace237eaea1d48cd81e94df Mon Sep 17 00:00:00 2001
>>> From: Greg Kroah-Hartman <gregkh@suse.de>
>>> Date: Tue, 16 Nov 2010 11:19:53 -0800
>>> Subject: Staging: iio: adis16220: fix up some sysfs attribute permissions
>>>
>>> They should not be writable by any user
>> Sorry Greg, it may be jet lag related, but I really don't see how this change
>> makes sense...  Doesn't this take a write only parameter and make it read only
>> with no read function defined?  Was the intent to restrict who could write
>> to this attribute?
> 
> Ah crap.
> 
>>>  #define IIO_DEV_ATTR_CAPTURE(_store)				\
>>> -	IIO_DEVICE_ATTR(capture, S_IWUGO, NULL, _store, 0)
>>> +	IIO_DEVICE_ATTR(capture, S_IRUSR, NULL, _store, 0)
> 
> This should be S_IWUSR, right?
Equivalent with that in and I'm happy to add my ack. 

Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

Thanks,

Jonathan
> 
> Did I just get all of these wrong?  I'll go verify them...
> 
> thanks,
> 
> greg k-h
> 

Re: patch "Staging: iio: adis16220: fix up some sysfs attribute permissions" added to staging tree

[Greg KH]

On Thu, Nov 18, 2010 at 05:54:18PM +0000, Jonathan Cameron wrote:
> On 11/18/10 17:19, Greg KH wrote:
> > On Thu, Nov 18, 2010 at 05:05:05PM +0000, Jonathan Cameron wrote:
> >> On 11/16/10 19:25, gregkh@suse.de wrote:
> >>>
> >>> This is a note to let you know that I've just added the patch titled
> >>>
> >>>     Staging: iio: adis16220: fix up some sysfs attribute permissions
> >>>
> >>> to my staging git tree which can be found at
> >>>     git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git
> >>> in the staging-linus branch.
> >>>
> >>> The patch will show up in the next release of the linux-next tree
> >>> (usually sometime within the next 24 hours during the week.)
> >>>
> >>> The patch will hopefully also will be merged in Linus's tree for the
> >>> next -rc kernel release.
> >>>
> >>> If you have any questions about this process, please let me know.
> >>>
> >>>
> >>> >From 1d904e8950c86e670ace237eaea1d48cd81e94df Mon Sep 17 00:00:00 2001
> >>> From: Greg Kroah-Hartman <gregkh@suse.de>
> >>> Date: Tue, 16 Nov 2010 11:19:53 -0800
> >>> Subject: Staging: iio: adis16220: fix up some sysfs attribute permissions
> >>>
> >>> They should not be writable by any user
> >> Sorry Greg, it may be jet lag related, but I really don't see how this change
> >> makes sense...  Doesn't this take a write only parameter and make it read only
> >> with no read function defined?  Was the intent to restrict who could write
> >> to this attribute?
> > 
> > Ah crap.
> > 
> >>>  #define IIO_DEV_ATTR_CAPTURE(_store)				\
> >>> -	IIO_DEVICE_ATTR(capture, S_IWUGO, NULL, _store, 0)
> >>> +	IIO_DEVICE_ATTR(capture, S_IRUSR, NULL, _store, 0)
> > 
> > This should be S_IWUSR, right?
> Equivalent with that in and I'm happy to add my ack. 
> 
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

Thanks.

And yes, I got all 7 of these patches wrong, I'll go redo them :(

thanks for verifying and actually reviewing them, I appreciate it.

greg k-h