linux-kernel - Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150423042300.GA724@swordfish>
Date:	Thu, 23 Apr 2015 13:23:00 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Minchan Kim <minchan@...nel.org>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

On (04/23/15 12:06), Minchan Kim wrote:
> > +Example:
> > +	cat /sys/class/zram-control/zram_add
> 
> Why do we put zram-contol there rather than /sys/block/zram

that's what clsss_register() does.

[..]

> > @@ -1168,8 +1172,15 @@ static int zram_add(int device_id)
> 
> Why do zram_add need device_id?
> We decided to remove option user pass device_id.

will cleanup. it was simpler at that time to support both devices
created by sysfs request and devices pre-crated for num_devices
module param.


> > +static struct zram *zram_lookup(int dev_id)
> > +{
> > +	struct zram *zram;
> > +
> > +	zram = idr_find(&zram_index_idr, dev_id);
> > +	if (zram)
> > +		return zram;
> > +	return ERR_PTR(-ENODEV);
> 
> Just return NULL which is more simple and readable.
> 

ok.


[..]
> > +	/*
> > +	 * First, make ->disksize device attr RO, closing
> > +	 * zram_remove() vs disksize_store() race window
> 
> Why don't you use zram->init_lock to protect the race?

zram_reset_device() takes this lock internally. but, it
unlocks the device upon the return from zram_reset_device():

lock idr_lock
 zram_reset_device()
  lock bd_mutex
    __zam_reset_device()
        lock init_lock
         reset
        unlock init_lock     ---\
  unlock bd_mutex                |
                                 |<----- disksize_store() race window
 zram_remove()               ---/
unlock idr_lock


until we call zram_remove() (which does sysfs_remove_group()) device has
sysfs attrs and, thus, disksize_store() can arrive in the middle. the
simplest things I came up with was that RO bit on sysfs disksize attrs.
I can factor out another set of __foo function to handle it differently,
not sure if this worth it.

I'll revisit it.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/