[PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[hujianyang]

CS node is recognized as a sign in UBIFS log replay mechanism.
Log relaying during mount should find the CS node in first log
leb at beginning and then replay the following uncommitted
buds.

Here is a bug in log replay path: If first log leb, which is
indicated by @log_lnum in mst_node, is empty, current UBIFS
replay nothing and directly mount the partition without any
warning. This action will put filesystem in an abnormal state,
e.g. space management in LPT area is incorrect to the real
space usage in main area.

We reproduced this bug by fault injection: turn first log leb
into all 0xFF. UBIFS driver mount the polluted partition
normally. But errors occur while running fs_stress on this
mount:

[89068.055183] UBI error: ubi_io_read: error -74 (ECC error) while reading 59 bytes from PEB 711:33088, read 59 bytes
[89068.179877] UBIFS error (pid 10517): ubifs_check_node: bad magic 0x101031, expected 0x6101831
[89068.179882] UBIFS error (pid 10517): ubifs_check_node: bad node at LEB 591:28992
[89068.179891] Not a node, first 24 bytes:
[89068.179892] 00000000: 31 10 10 00 37 84 64 04 10 04 00 00 00 00 00 00 20 00 00 00 02 01 00 00                          1...7.d......... .......
[89068.180282] UBIFS error (pid 10517): ubifs_read_node: expected node type 2

This patch fix the problem by checking c->cs_sqnum to
guarantee the empty leb is not first log leb and return an
error if the first log leb is incorrectly empty. After this,
we could catch *first log empty* error in place.

Signed-off-by: hujianyang <hujianyang@huawei.com>
---
 fs/ubifs/replay.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c
index 3187925..f13f4b2 100644
--- a/fs/ubifs/replay.c
+++ b/fs/ubifs/replay.c
@@ -846,7 +846,14 @@ static int replay_log_leb(struct ubifs_info *c, int lnum, int offs, void *sbuf)
 	}

 	if (sleb->nodes_cnt == 0) {
-		err = 1;
+		if (unlikely(c->cs_sqnum == 0)) {
+			/* This is the first log LEB, should not be empty */
+			ubifs_err("first log leb LEB %d:%d is empty, no CS node exist",
+				  lnum, offs);
+			err = -EINVAL;
+		} else {
+			err = 1;
+		}
 		goto out;
 	}

-- 
1.6.0.2

Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[Artem Bityutskiy]

Hi Hujianyang,

On Sat, 2015-01-31 at 11:43 +0800, hujianyang wrote:
> CS node is recognized as a sign in UBIFS log replay mechanism.
> Log relaying during mount should find the CS node in first log
> leb at beginning and then replay the following uncommitted
> buds.

Lets use the "log head" term instead of "the first log LEB", just to be
consistent.

> Here is a bug in log replay path: If first log leb, which is
> indicated by @log_lnum in mst_node, is empty, current UBIFS
> replay nothing and directly mount the partition without any
> warning. This action will put filesystem in an abnormal state,
> e.g. space management in LPT area is incorrect to the real
> space usage in main area.

Looks like a good catch, thank you! I have few requests, though.

> 
>  	if (sleb->nodes_cnt == 0) {
> -		err = 1;
> +		if (unlikely(c->cs_sqnum == 0)) {

Please, do not use unlikely(), it may only be used on hot-paths, which
the replay code is certainly not.

IIUC, this if translates to "is this the head of the log?". If I am
right, then the better place for doing this check is the caller function
- 'ubifs_replay_journal()'. In that function you just do

if (lnum == c->lhead_lnum)

which is way easier to understand than "if (c->cs_sqnum == 0)", I think.

> +			/* This is the first log LEB, should not be empty */

I'd expand this comment a bit to:

The head of the log must always start with the "commit start" node on a
properly formatted UBIFS. But we found no nodes at all, which means that
something went wrong and we cannot proceed mounting the file-system.

I think the next developer who looks at the will appreciate these kinds
of commentaries.

> +			ubifs_err("first log leb LEB %d:%d is empty, no CS node exist",
> +				  lnum, offs);

Lets use consistent terminology. We call it the "log head".

I'd phrase it like this:

no UBIFS nodes found at the log head LEB %d:%d, possibly the FS is
corrupted.

Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[hujianyang]

On 2015/1/31 17:51, Artem Bityutskiy wrote:
> Hi Hujianyang,
> 
> On Sat, 2015-01-31 at 11:43 +0800, hujianyang wrote:
>> CS node is recognized as a sign in UBIFS log replay mechanism.
>> Log relaying during mount should find the CS node in first log
>> leb at beginning and then replay the following uncommitted
>> buds.
> 
> Lets use the "log head" term instead of "the first log LEB", just to be
> consistent.
>

OK.

>> +		if (unlikely(c->cs_sqnum == 0)) {
> 
> Please, do not use unlikely(), it may only be used on hot-paths, which
> the replay code is certainly not.
> 
> IIUC, this if translates to "is this the head of the log?". If I am
> right, then the better place for doing this check is the caller function
> - 'ubifs_replay_journal()'. In that function you just do
> 
> if (lnum == c->lhead_lnum)
> 
> which is way easier to understand than "if (c->cs_sqnum == 0)", I think.
> 

That's true~!

>> +			/* This is the first log LEB, should not be empty */
> 
> I'd expand this comment a bit to:
> 
> The head of the log must always start with the "commit start" node on a
> properly formatted UBIFS. But we found no nodes at all, which means that
> something went wrong and we cannot proceed mounting the file-system.
> 

Thanks~!

> I think the next developer who looks at the will appreciate these kinds
> of commentaries.
> 
>> +			ubifs_err("first log leb LEB %d:%d is empty, no CS node exist",
>> +				  lnum, offs);
> 
> Lets use consistent terminology. We call it the "log head".
> 
> I'd phrase it like this:
> 
> no UBIFS nodes found at the log head LEB %d:%d, possibly the FS is
> corrupted.
> 

Artem, Thanks for your reviewing. Could I add a Reviewed-by after resending
a v2 patch?

Sorry to say I'm too tired today and still have other stuff needs to be
done before my leave. I'd like to recheck your comments and resend this
patch next Monday.

Thank you very much~!
Hu

Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[Artem Bityutskiy]

On Sat, 2015-01-31 at 18:34 +0800, hujianyang wrote:
> Artem, Thanks for your reviewing. Could I add a Reviewed-by after resending
> a v2 patch?

Well, I do want to help you to have things done quicker by letting you
adding the Reviewed-by tag already in v2. But the protocol is the
protocol, the Reviewed-by tag is usually something the reviewer replies
with when he sees the final version of the patch.

Besides, I am still the one who merges UBIFS stuff. I am letting Richard
to be the UBI master so far, since this is the area he demonstrated the
excellence at, not UBIFS.

UBIFS and UBI are very different, just like MTD and UBI. Knowing one of
them well does not make you automatically fluent in the other.

So the layout I proposed Richard was:

1. Richard owns UBI, I most probably will slowly shade away from that
area.
2. Richard owns mtd-utils
3. I still own UBIFS, but will be ready to hand it over to the next
brave one.

<sentiment>
UBI is my baby, it is my first serious piece of work I've done. Working
with smart people from Linutronix, for IBM was fantastic!

But UBI is "grown up" and it is time for me to move forward. I am very
happy to have Richard to take it over, thanks Richard. Maintain it
smartly, make sure your users are happy.

I will be glad to help with architectural/design things. Whenever there
is a debate, or a need in an educated opinion, I'll be happy to give my
input.
</sentiment>

Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[hujianyang]

On 2015/1/31 20:16, Artem Bityutskiy wrote:
> On Sat, 2015-01-31 at 18:34 +0800, hujianyang wrote:
>> Artem, Thanks for your reviewing. Could I add a Reviewed-by after resending
>> a v2 patch?
> 
> Well, I do want to help you to have things done quicker by letting you
> adding the Reviewed-by tag already in v2. But the protocol is the
> protocol, the Reviewed-by tag is usually something the reviewer replies
> with when he sees the final version of the patch.
>

Sure. I just thought it's a simple problem and I could fix it as you
suggested in v2. Since I'm not a careful man, I wish you could keep
on commenting my v2 patch.

Thanks for your help in this patch again.

> Besides, I am still the one who merges UBIFS stuff. I am letting Richard
> to be the UBI master so far, since this is the area he demonstrated the
> excellence at, not UBIFS.
> 
> UBIFS and UBI are very different, just like MTD and UBI. Knowing one of
> them well does not make you automatically fluent in the other.
> 
> So the layout I proposed Richard was:
> 
> 1. Richard owns UBI, I most probably will slowly shade away from that
> area.
> 2. Richard owns mtd-utils
> 3. I still own UBIFS, but will be ready to hand it over to the next
> brave one.
> 

I think you are busy with other stuff these days. I'm sorry to disturb
you a lot.

> <sentiment>
> UBI is my baby, it is my first serious piece of work I've done. Working
> with smart people from Linutronix, for IBM was fantastic!
> 
> But UBI is "grown up" and it is time for me to move forward. I am very
> happy to have Richard to take it over, thanks Richard. Maintain it
> smartly, make sure your users are happy.
> 
> I will be glad to help with architectural/design things. Whenever there
> is a debate, or a need in an educated opinion, I'll be happy to give my
> input.
> </sentiment>
> 
> 
> 

Thanks~!

Another problem. Someone <markus.heininger@online.de> asked:

"UBIFS: Is it possible to get the compressed size of a file?"

I think it's an interesting problem. Actually we used to separate an UBI
device into several volumes for space management reason. So could we
realize the compressed size getting mechanism and import *quota* to UBIFS
itself.

BR,
Hu

Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[Richard Weinberger]

Am 02.02.2015 um 04:10 schrieb hujianyang:
> Another problem. Someone <markus.heininger@online.de> asked:
> 
> "UBIFS: Is it possible to get the compressed size of a file?"
> 
> I think it's an interesting problem. Actually we used to separate an UBI
> device into several volumes for space management reason. So could we
> realize the compressed size getting mechanism and import *quota* to UBIFS
> itself.

You can get the actual size by accumulating the UBIFS data node sizes.
But this means that you have to load the actual data node which is rather
expensive. I had a short chat with Artem on this, we agreed that a nice solution
would be to store the actual size in an inode. But that is a non-trivial change
to UBIFS.

Thanks,
//richard

Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[hujianyang]

Hi Richard,

On 2015/2/2 16:20, Richard Weinberger wrote:
> Am 02.02.2015 um 04:10 schrieb hujianyang:
>> Another problem. Someone <markus.heininger@online.de> asked:
>>
>> "UBIFS: Is it possible to get the compressed size of a file?"
>>
>> I think it's an interesting problem. Actually we used to separate an UBI
>> device into several volumes for space management reason. So could we
>> realize the compressed size getting mechanism and import *quota* to UBIFS
>> itself.
> 
> You can get the actual size by accumulating the UBIFS data node sizes.

and xattr node sizes too? may store in another variable.

> But this means that you have to load the actual data node which is rather
> expensive. I had a short chat with Artem on this, we agreed that a nice solution
> would be to store the actual size in an inode. But that is a non-trivial change
> to UBIFS.
> 

Yes, old images may not store the actual size and it's hard to
get compressed size from this kind of images by a new driver.

Users may get actual size from an ioctl call and maybe we could
return an error if this feature is not supported. How about
increasing file-system driver version and applying actual size
support in a higher version? or mark this feature as a flag
in super_node?

Another 'expensive' implement is whole scan the partition to
get the actual size of a required file in the old images.

What's your plan after that chat?


BR,
Hu