linux-kernel - Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL shutdown

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: <1412063620.2379.12.camel@sauron.fi.intel.com>
Date:	Tue, 30 Sep 2014 07:53:40 +0000
From:	"Bityutskiy, Artem" <artem.bityutskiy@...el.com>
To:	"richard@....at" <richard@....at>
CC:	"dedekind1@...il.com" <dedekind1@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH 1/4] UBI: Ensure that all fastmap work is done upon WL
 shutdown

On Tue, 2014-09-30 at 08:58 +0200, Richard Weinberger wrote:
> Am 30.09.2014 08:26, schrieb Artem Bityutskiy:
> > On Tue, 2014-09-30 at 00:20 +0200, Richard Weinberger wrote:
> >> ...otherwise the deferred work might run after datastructures
> >> got freed and corrupt memory.
> > 
> > How can this happend? The background thread is stopped by this time
> > already, so what are the other possibilities? And why is this
> > fastmap-only?
> 
> This has nothing do to with the background thread.
> Fastmap has a work queue. If one fastmap work has been
> scheuled we have to wait for it.

I expected a bit more explanation. But OK, here is what I think.

UBI consists of subsystems. Subsystems try to be more or less
independent, whenever possible. They expose interface functions for
other subsystems. Of course the split is not ideal, but we do our best.

* wl.c does wear-levelling.
* wl.c does not do fastmap.
* fastmap.c does fastmap.
* I am unhappy seeing yet another ifdef to wl.c
* I am unhappy seeing wl.c calling 'flush_work(&ubi->fm_work)', because
fastmap.c should deal with 'fm_work'. Or said differently, wl.c is not a
fastmap queue baby-sitter. fastmap.c is.

Most UB subsystems have the init and close function. May be adding one
for fastmap would help? Then you could flush whatever from
'ubi_wl_close()' ?

Historically the work queue was implemented in wl.c because wl.c was the
only user of it.

If this layout is not good enough, we should probably extend it, may be
separate work queue management out of wl.c.

But populating wl.c with macros and little "take care of this fatmap
bit" stuff is a not going to lead to better code structure.

-- 
Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.