linux-kernel - Re: [PATCH] UBIFS: use ubi's new ubi_leb

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] [day] [month] [year] [list]

Message-ID: <CAF4G-tKbm-f84+8-zcAO60jvqhrv-jG90P=2o+J6PByvfQY7Rw@mail.gmail.com>
Date:	Thu, 12 Apr 2012 14:17:30 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Joel Reardon <joel@...mbassador.com>
Cc:	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] UBIFS: use ubi's new ubi_leb_change sync parameter

Hi Joel,

let's not CC fsdevel anymore so far - we generate too much traffic there.

On Wed, Apr 11, 2012 at 6:38 PM, Joel Reardon <joel@...mbassador.com> wrote:
> This patch fixes UBIFS to use the new sync parameter for ubi's ubi_leb_change
> function. In the previous post, one of the calls had sync = 1, this is
> fixed. This, along with the ubi patch that introduces the new
> parameter, was tested using integck for both sync=0 and sync=1 in the
> tnc_commit's call to leb_change and the underlying free'd PEB was
> inspected through debug statements to ensure that it was later reallocated
> for new data.

I was thinking about this again and I do not really like the patch
anymore because it
does not solve the problem.

Indeed, we have the wear-levelling subsystem which may decide at any point that
the contents of one of the PEBs containing keys has to be moved to a
different PEB.
It will move it and then schedule the old PEB for erasure. Your
solution does not guarantee
that this old PEB is now erased. And thus, you do not achieve what you
want to achieve.

First of all, notice, that you can work on this aspect independently
of the UBIFS part.

I guess you actually have 2 choices.

1. Flush entire erasblocks queue.
2. Implement a funtion which will flush the queue only for a specific LEB.

The first approach may be very slow, especially on NOR. Also, I've
just noticed that
ubi_sync() does not actually do this - it only calls 'mtd_sync()'. You
need to introduce a
parameter to mtd_sync() which will tell whether to only sync the MTD
device or also
flush the entire queue. This is trivial. To flush the queue, you need
to call 'ubi_wl_flush()'.

The second approach is also not too difficult to do, I think.

Basically, you add a 'int lnum' parameter to 'mtd_sync()'. Then you
add this parameter
to 'ubi_wl_flush()', and may be to 'do_work()' as well.

Then you need to make sure you have 'lnum' available in the elements
of the 'ubi->works'
list. This basically means you need to have an 'int lnum' field in
'struct ubi_work'. Probably
this means that 'schedule_work()' should also accept an 'int lnum'
argument. Does not
sound too difficult.

Then you will be able to achieve what you want by calling
'ubi_leb_change()' first, and
then 'ubi_sync(lnum)'.

Also note, after mount you also have to call 'ubi_sync(lnum)' for all
LEBs containing
the keys. This is because you may have an unclean reboot just before
you have erased
the old PEB.

So, I am sorry, but I am removing so far this patch from my tree.

What do you think?

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)
--
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/