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
| ||
|
Date: Tue, 3 Nov 2015 09:18:35 +0900 From: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com> To: Geliang Tang <geliangtang@....com> Cc: Minchan Kim <minchan@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, Sergey Senozhatsky <sergey.senozhatsky@...il.com>, Sergey Senozhatsky <sergey.senozhatsky.work@...il.com> Subject: Re: [PATCH] zram: move init_done check to the beginning of disksize_store Hi, [please don't trim the Cc list] On (11/02/15 21:53), Geliang Tang wrote: > > Those things are not connected, absolutely. zram_meta_alloc() can fail > > even on un-initialized device. In any case disksize_store() does not end > > up changing the state of the device and reports the error back, so I don't > > see any real value in this change. Seems that we come across this change > > something like once a year (this patch is not the first). > > Thanks for your reply. > > zram_meta_alloc() always fails on initialized device at debugfs_create_dir(): > debugfs dir <zram0> creation failed Aha, so you are talking about calling disksize_store() on - already initialized device that - uses zsmalloc with - CONFIG_ZSMALLOC_STAT as an alacator (zbud is not affected). that should have been mentioned in the commit message. hm, does the patch _actually_ make it better? splitting a ->init_lock regions is *never* a good idea. consider an un-initialized device. CPU0 CPU1 down_read(&zram->init_lock); if (init_done(zram)) return -EBUSY; up_read(&zram->init_lock); down_read(&zram->init_lock); if (init_done(zram)) return -EBUSY; up_read(&zram->init_lock); meta = zram_meta_alloc(); meta = zram_meta_alloc(); comp = zcomp_create(); comp = zcomp_create(); down_write(&zram->init_lock); zram->meta = meta; ... up_write(&zram->init_lock); down_write(&zram->init_lock); zram->meta = meta; ... up_write(&zram->init_lock); So the above code will return a "debugfs dir <zram0> creation failed" IFF zsmalloc with CONFIG_ZSMALLOC_STAT enabled is being used (regardless the effort). And it will totally succeed and lead to N (depending on the number of concurrent disksize_store() calls) pool memory and zcomp_create memory leaks otherwise. Because of this part: --- down_write(&zram->init_lock); - if (init_done(zram)) { - pr_info("Cannot change disksize for initialized device\n"); - err = -EBUSY; - goto out_destroy_comp; - } - init_waitqueue_head(&zram->io_done); atomic_set(&zram->refcount, 1); zram->meta = meta; --- > zram: Error creating memory pool > > I think we should deal with this error. Is it so critical? I mean it does not break the existing device; we print the error and keep the existing device fully functional. The error line may be a bit misleading, which is another thing and can be solved documenting the behavior, *for example*. (I just don't really want to complicate the code for a corner case which doesn't even break anything). -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/
Powered by blists - more mailing lists