Re: [PATCH] zram: restrict add/remove attributes to root only

[Greg KH]

On Sun, Dec 04, 2016 at 08:41:17PM +0900, Sergey Senozhatsky wrote:
> On (12/04/16 12:28), Greg KH wrote:
> > Date: Sun, 4 Dec 2016 12:28:20 +0100
> > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > To: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>, Minchan Kim
> >  <minchan@xxxxxxxxxx>, Steven Allen <steven@xxxxxxxxxxxxx>,
> >  linux-kernel@xxxxxxxxxxxxxxx, stable@xxxxxxxxxxxxxxx, Sergey Senozhatsky
> >  <sergey.senozhatsky.work@xxxxxxxxx>
> > Subject: Re: [PATCH] zram: restrict add/remove attributes to root only
> > User-Agent: Mutt/1.7.1 (2016-10-04)
> > 
> > On Sun, Dec 04, 2016 at 07:52:08PM +0900, Sergey Senozhatsky wrote:
> > > On (12/04/16 11:28), Greg KH wrote:
> > > > On Sun, Dec 04, 2016 at 11:35:15AM +0900, Sergey Senozhatsky wrote:
> > > [..]
> > > 
> > > > Why can't a normal user read the attribute?  Does a read actually modify
> > > > something?
> > > 
> > > yes, it does.
> 
> to clarify a bit more:
> 
> we allocate a new device ID using idr_alloc(). so the IDs are limited
> and, thus, the number of devices is limited as well - signed int. each
> new device has NO:
>  -- zspoll (zsmalloc pool in zram case)
>  -- compression per-CPU backends (working-mem/scratch buffers, etc.)
>  -- meta table
> 
> so no big memory allocations. (a 'normal' user can't init the device,
> he/she can just create it. which is the problem here: we don't want a
> 'normal' user be able to do this).
> 
> every device has:
>  -- blk queue
>  -- sysfs attrs
>  -- gendisk
>  -- zram structure allocated.
> 
> so each new device consumes some memory, but not insane amounts of it.

That's fine, the issue is that reading a file should not cause the
system state to change.  That's just not a logical thing to have happen,
no other sysfs files do that.  Why is zram "special" in this way?

> > Oh that's totally and completely broken then.
> > 
> > Reading from a sysfs file should NEVER cause side affects to the system.
> > Please fix up this api.
> 
> some history. we started with a 'loop device'-like scheme, but
> ended up with a sysfs approach
> 
>  [1] https://marc.info/?l=linux-kernel&m=142495984002611
>  [2] https://marc.info/?l=linux-kernel&m=142507747808572
>  [3] https://marc.info/?l=linux-kernel&m=142530591720172
>  [4] https://marc.info/?l=linux-kernel&m=142509446812318
>  [5] https://marc.info/?l=linux-kernel&m=142509782112819

you should have stuck with the "write a value to the sysfs file" api,
for some reason that didn't stick...

> > > reading from a hot_add file creates a new zram device and returns a new
> > > device's device_id. not initialized device (so it does not eat the memory
> > > for handle table, etc.), but with its own set of sysfs attrs, etc. which
> > > consumes memory after all. so a 'normal' user, doing a simple read from a
> > > hot_add file in a loop just for fun, can create a lot of devices and,
> > > quite likely, cause some troubles (as reported by Steven Allen).
> > 
> > Please switch this to be a char device node if you wish to "write and
> > get a device handle back".  I don't know how I missed that in the
> > original api review, sorry about that.
> >
> > For now, you need to document the heck out of this in the attribute
> > declaration that this is what is going on.  Otherwise someone like me
> > will come along and "fix up" the file to use ATTR_RO again in the
> > future and you will have the same problem again.
> 
> 
> I believe we have a documentation
> 
> 	Documentation/ABI/testing/sysfs-class-zram
> and
> 	Documentation/blockdev/zram.txt
> 
> both explain this attr.

Yes, but that's not in the code itself.  You are doing something VERY
different here than any other sysfs file.  The code better explain it
very well so that I don't go and change this back sometime in the future
when I sweep the kernel for "odd sysfs mode values" like I do every few
years.

So comment this please, why would you object to that?

greg k-h