JFFS2 bugfix

JFFS2 bugfix

[Artem B. Bityuckiy]

Hello,

I've found bug in JFFS2. When there is no free space left on JFFS2 file 
system, and somebody for example tries to creade new directory, JFFS2 
frees memory twice.

See dir.c, jffs2_create(), line 216.

jffs2_do_create returns error.
jffs2_clear_inode(inode) is called and frees the jffs2_sb_info, 
jffs2_full_dnode, etc.
iput(inode) is called, calling in turn the jffs2_clear_inode, and the 
same structures are freed for the second time. This leads to the slab 
cache corruption.

So, the following patch which fixes the problem is proposed.

--- dir.c       2004-10-16 21:02:22.886276648 +0400
+++ dir_corrected.c     2004-10-16 21:03:10.843766654 +0400
@@ -217,7 +217,6 @@
                               dentry->d_name.name, dentry->d_name.len);

         if (ret) {
-               jffs2_clear_inode(inode);
                 make_bad_inode(inode);
                 iput(inode);
                 jffs2_free_raw_inode(ri);

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 bugfix

[Ferenc Havasi]

Hi Artem,

I tried reproduce your bug:

 > I've found bug in JFFS2. When there is no free space left on JFFS2
 > file system, and somebody for example tries to creade new directory,
 > JFFS2 frees memory twice.
 >
 > See dir.c, jffs2_create(), line 216.
 >
 > jffs2_do_create returns error.
 > jffs2_clear_inode(inode) is called and frees the jffs2_sb_info,
 > jffs2_full_dnode, etc.
 > iput(inode) is called, calling in turn the jffs2_clear_inode, and the
 > same structures are freed for the second time. This leads to the slab
 > cache corruption.

I was not successful. I write JFFS2 to full (there was no left space),
than I tried to mkdir, but there was no slab cache corruption.

 > --- dir.c       2004-10-16 21:02:22.886276648 +0400
 > +++ dir_corrected.c     2004-10-16 21:03:10.843766654 +0400
 > @@ -217,7 +217,6 @@
 >                               dentry->d_name.name, dentry->d_name.len);
 >
 >         if (ret) {
 > -               jffs2_clear_inode(inode);
 >                 make_bad_inode(inode);
 >                 iput(inode);
 >                 jffs2_free_raw_inode(ri);

If I know well now jffs2_clear_inode only try to free only the fragtree,
its dirents... but the inode is still present, and need to handle.

Maybe I am wrong. I am not very familiar with this part of JFFS2.
Are you sure that this is really a bug?

Regards,
Ferenc

Re: JFFS2 bugfix

[Artem B. Bityuckiy]

Hello Ferenc,

Ferenc Havasi wrote:
> Hi Artem,
> 
> I tried reproduce your bug:
> 
>  > I've found bug in JFFS2. When there is no free space left on JFFS2
>  > file system, and somebody for example tries to creade new directory,
>  > JFFS2 frees memory twice.
>  >
>  > See dir.c, jffs2_create(), line 216.
>  >
>  > jffs2_do_create returns error.
>  > jffs2_clear_inode(inode) is called and frees the jffs2_sb_info,
>  > jffs2_full_dnode, etc.
>  > iput(inode) is called, calling in turn the jffs2_clear_inode, and the
>  > same structures are freed for the second time. This leads to the slab
>  > cache corruption.
> 
> I was not successful. I write JFFS2 to full (there was no left space),
> than I tried to mkdir, but there was no slab cache corruption.
Did you see system message "No space left on device"?

How did you see that slab is OK? I've found this when I've enabled the 
correspondent option in "Linux Hacking" (no sure, possibly 
CONFIG_DEBUG_SLAB).

> 
>  > --- dir.c       2004-10-16 21:02:22.886276648 +0400
>  > +++ dir_corrected.c     2004-10-16 21:03:10.843766654 +0400
>  > @@ -217,7 +217,6 @@
>  >                               dentry->d_name.name, dentry->d_name.len);
>  >
>  >         if (ret) {
>  > -               jffs2_clear_inode(inode);
>  >                 make_bad_inode(inode);
>  >                 iput(inode);
>  >                 jffs2_free_raw_inode(ri);
> 
> If I know well now jffs2_clear_inode only try to free only the fragtree,
> its dirents... but the inode is still present, and need to handle.
Yes, it does. But after this the iput() function is called. The iput() 
calls jffs2_clear_inode too. And the same data structures (fragtrees 
with full_dnodes, direntries) are freed one more time. This is not good :-)
> 
> Maybe I am wrong. I am not very familiar with this part of JFFS2.
> Are you sure that this is really a bug?
Hmm. Yes, I think so... May be small one :-)

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 bugfix

[Ferenc Havasi]

Hi Artem,

>> I was not successful. I write JFFS2 to full (there was no left space),
>> than I tried to mkdir, but there was no slab cache corruption.
> 
> Did you see system message "No space left on device"?
> 
> How did you see that slab is OK? I've found this when I've enabled the 
> correspondent option in "Linux Hacking" (no sure, possibly 
> CONFIG_DEBUG_SLAB).

Yes, I've got the message "No space left on device", and I've 
CONFIG_DEBUG_SLAB enabled.

I didn't got slab cache error because it has already returned with "no 
left space" before this place. But, I think you are right, and I 
commited your patch.

I hope David will aggree with it.

Bye,
Ferenc

Re: JFFS2 bugfix

[Artem B. Bityuckiy]

Hello Ferenc,

> Hi Artem,
> 
> I didn't got slab cache error because it has already returned with "no 
> left space" before this place. But, I think you are right, and I 
> commited your patch.
Don't understand exactly what does "it has already returned with "no 
left space" before this place" mean (which place?), but anyway, you have 
recognized the problem.

> I hope David will aggree with it.
I hope either :-)

Thank you.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 bugfix

[David Woodhouse]

On Tue, 2004-10-19 at 09:57 +0200, Ferenc Havasi wrote:
>  But, I think you are right, and I commited your patch.
> 
> I hope David will aggree with it.

The patch looks correct. We should probably double-check the other error
cases which behave similarly -- I think there are cases were we should
be doing an iput() but we don't.

-- 
dwmw2

Re: JFFS2 compression

[Ferenc Havasi]

Hi Artem,

> Just wonder, why the compression mode is the global variable 
> (jffs2_get_compression_mode, defined in compr.c). Theoretically, it 
> should be the attribute of superblock (in this case it would be possible 
> to have several JFFS2 filesystems with different compression modes).

Now the compression mode is a global thing. It can be configured by 
kernelconfig and is the same for every filesystem. There is no reserved 
space to store it in the different JFFS2 filesystems (it should need a 
new type of node or something like that).

There is plans for the future. One of them: there will be "compression 
configurations". This is much more general thing than compression mode. 
(contais not only the compression mode but the set of the enabled 
compressors and its priority) The user will be able to define different 
compressor configuration for files or directories. The description of 
these configuration will be stored in a new type of node 
(RWCOMPACT_COPY), and the index of the requested configuration will be 
stored in the upper 4 bits of 'usercompr' field of jffs2_raw_inode.

But it is only a plan. In mkfs it is already impemented (not commited 
yet), but in the file system for first we plan only to implement a very 
limited version of it (which support only the ability to declare a 
directory(s) for no compression).

Bye,
Ferenc

Re: JFFS2 compression

[David Woodhouse]

On Tue, 2004-10-19 at 12:09 +0200, Ferenc Havasi wrote:
> Hi Artem,
> 
> > Just wonder, why the compression mode is the global variable 
> > (jffs2_get_compression_mode, defined in compr.c). Theoretically, it 
> > should be the attribute of superblock (in this case it would be possible 
> > to have several JFFS2 filesystems with different compression modes).
> 
> Now the compression mode is a global thing. It can be configured by 
> kernelconfig and is the same for every filesystem. There is no reserved 
> space to store it in the different JFFS2 filesystems (it should need a 
> new type of node or something like that).
> 
> There is plans for the future. One of them: there will be "compression 
> configurations". This is much more general thing than compression mode. 

Increasingly, I think I'd like to see the compression code taken out of
JFFS2 and moved somewhere where other things in the kernel can use it.
While we had only a few local hacks it made a certain amount of sense
for it to be kept private, but now it's getting a lot more useful, I
don't think it should be limited to JFFS2.

We should probably plug it in using the crypto API or something similar.
There's not a _huge_ amount of difference between compression routines
and crypto routines. 

-- 
dwmw2

Re: JFFS2 compression

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> Now the compression mode is a global thing. It can be configured by 
> kernelconfig and is the same for every filesystem. There is no reserved 
> space to store it in the different JFFS2 filesystems (it should need a 
> new type of node or something like that).
It isn't needed to store the default compression mode *on flash*. If you 
just move the variable jffs2_compression mode to the jffs2_sb_info 
structure, you will have per-filesystems compression mode. This means, 
for example, that if user changes (dynamically) the compression mode for 
one file-system, the compression mode for another file system isn't changed.

> 
> There is plans for the future. One of them: there will be "compression 
> configurations". This is much more general thing than compression mode. 
> (contais not only the compression mode but the set of the enabled 
> compressors and its priority) The user will be able to define different 
> compressor configuration for files or directories. The description of 
> these configuration will be stored in a new type of node 
> (RWCOMPACT_COPY), and the index of the requested configuration will be 
> stored in the upper 4 bits of 'usercompr' field of jffs2_raw_inode.
Great idea!

But this is the another (deeper) layer. If a file/directory has the 
compression mode set, it would be applied. But the *per-filesystem* 
compression may be used by default (for those files who are not 
mentioned in your "compression configuration" node).



-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 compression

[Ferenc Havasi]

Artem B. Bityuckiy wrote:
> It isn't needed to store the default compression mode *on flash*. If you 
> just move the variable jffs2_compression mode to the jffs2_sb_info 
> structure, you will have per-filesystems compression mode. This means, 
> for example, that if user changes (dynamically) the compression mode for 
> one file-system, the compression mode for another file system isn't 
> changed.

I undersand you, but unfortunatelly now there is no way to change 
compression mode dimamically. There was (using procfs) but when David 
tried to commit it to the kernel, someone (I don't remember who but one 
of the kernel developers) declared it bad.

David asked me to reimplement it is sysfs, but that is not an easy 
thing. Not technically. There is no fs subdirectory in sysfs. I read in 
the mailling list archive of linux-fsdevel@vger.kernel.org. Someone 
tried to submit a patch to make this fs subdirectory, because he need it 
for NFS. The answer declared it as very bad, too... (sysfs has a 
different philosopy...) I had some private discussion with the patch 
submitter (what happened exactly, what was really the problem,... what 
is his plan). He wrote that he try it again later. He think the guy who 
declared his patch bad really hates sysfs (and procfs too). So I decided 
to wait, too. :(

But this new "compressoin configuration" concept may cause that we don't 
need sysfs/procfs.

Bye,
Ferenc

Re: JFFS2 compression

[Artem B. Bityuckiy]

> I undersand you, but unfortunatelly now there is no way to change 
> compression mode dimamically. There was (using procfs) but when David 
> tried to commit it to the kernel, someone (I don't remember who but one 
> of the kernel developers) declared it bad.
> 
> David asked me to reimplement it is sysfs, but that is not an easy 
> thing. Not technically. There is no fs subdirectory in sysfs. I read in 
> the mailling list archive of linux-fsdevel@vger.kernel.org. Someone 
> tried to submit a patch to make this fs subdirectory, because he need it 
> for NFS. The answer declared it as very bad, too... (sysfs has a 
> different philosopy...) I had some private discussion with the patch 
> submitter (what happened exactly, what was really the problem,... what 
> is his plan). He wrote that he try it again later. He think the guy who 
> declared his patch bad really hates sysfs (and procfs too). So I decided 
> to wait, too. :(
Ferenc, thanks for story, I really interested why the proc support was 
removed. There is another possibility - ioctl/fcntl. Ok, got it - it is 
not big deal to move compression mode to superblock, but now it is just 
unneeded because there are no interfaces to change the compression mode 
dynamically.
> 
> But this new "compressoin configuration" concept may cause that we don't 
> need sysfs/procfs.
How are you going to set files/directories compression mode (just 
interesting) ?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 compression

[David Woodhouse]

On Tue, 2004-10-19 at 16:19 +0400, Artem B. Bityuckiy wrote:
> Ferenc, thanks for story, I really interested why the proc support was 
> removed. 

It was buggy. There were buffer overflows and similar problems with it.
I dropped it and suggested that the reimplementation may as well use
sysfs.

-- 
dwmw2

Re: JFFS2 compression

[Ferenc Havasi]

David Woodhouse wrote:
>>Ferenc, thanks for story, I really interested why the proc support was 
>>removed. 
> 
> It was buggy. There were buffer overflows and similar problems with it.
> I dropped it and suggested that the reimplementation may as well use
> sysfs.

Ups, than I misunderstood you.

Good to know that the problem was not the using of procfs itself, just 
my implementation. (it is much easier to fix :) )

Bye,
Ferenc

Re: JFFS2 compression

[David Woodhouse]

On Tue, 2004-10-19 at 16:07 +0200, Ferenc Havasi wrote:
> David Woodhouse wrote:
> >>Ferenc, thanks for story, I really interested why the proc support was 
> >>removed. 
> > 
> > It was buggy. There were buffer overflows and similar problems with it.
> > I dropped it and suggested that the reimplementation may as well use
> > sysfs.
> 
> Ups, than I misunderstood you.
> 
> Good to know that the problem was not the using of procfs itself, just 
> my implementation. (it is much easier to fix :) )

Using procfs is _also_ a slight problem -- it would be nicer to use
sysfs. But that's a minor problem not a reason not to merge it.

-- 
dwmw2

Re: JFFS2 compression

[David Woodhouse]

On Wed, 2004-10-20 at 11:16 +0200, Ferenc Havasi wrote:
> David Woodhouse wrote:
> 
> > We should probably plug it in using the crypto API or something similar.
> > There's not a _huge_ amount of difference between compression routines
> > and crypto routines. 
> 
> Yes, I agree with you. I will look after how cryto API works and think 
> over the differences between crypt and compression. It whould be also 
> nice to collect some really well working public compressor modul.

Thanks. Separating the compression stuff out from JFFS2 will also make
me less unhappy about the fact that some of it is only GPL'd and not
compatible with the eCos licence.

-- 
dwmw2

Re: JFFS2 compression

[Ferenc Havasi]

David Woodhouse wrote:

> We should probably plug it in using the crypto API or something similar.
> There's not a _huge_ amount of difference between compression routines
> and crypto routines. 

Yes, I agree with you. I will look after how cryto API works and think 
over the differences between crypt and compression. It whould be also 
nice to collect some really well working public compressor modul.

Regards,
Ferenc

Re: JFFS2 compression

[Artem B. Bityuckiy]

Hello Ferenc,

I know you are very familiar with compression in JFFS2. Please, could 
you answer me a question.

I have several different buffers. I want to compose one JFFS2 node from 
them. As I understand, I need to allocate one big temporary buffer, copy 
my small buffers to it, and then compress the big buffer. After this I 
will be able to uncompress the compression buffer in one step.

But, it would be very very nice if I can compress each small buffer to 
one compression buffer and later be able to uncompress the resulting 
compression buffer in one step. Can I do so?

Thanks.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 compression

[Ferenc Havasi]

Hi Artem,

> I have several different buffers. I want to compose one JFFS2 node from 
> them. As I understand, I need to allocate one big temporary buffer, copy 
> my small buffers to it, and then compress the big buffer. After this I 
> will be able to uncompress the compression buffer in one step.
> 
> But, it would be very very nice if I can compress each small buffer to 
> one compression buffer and later be able to uncompress the resulting 
> compression buffer in one step. Can I do so?

If the "one step" means that you would like to use only one 
jffs2_decompress/... call than the answer is no.

But I can imagine a solution, where you can call jffs2_compress 
separatedly for the small buffers, and you can uncompress it using a loop.

The compression buffer can be something like the following:
   - original_size_of_small_buffer1
   - compressed_size_of_small_buffer1
   - comprtype (return value of jffs2_compress)
   - compressed data1
   - original_size_of_small_buffer2
   - compressed_size_of_small_buffer2
   - comprtype (return value of jffs2_compress)
   - compressed data2
   ...

I don't know it is OK for you or not.

Bye,
Ferenc

P.S.: In this solution you may have to allign all data to 4 bytes.

Re: JFFS2 compression

[Artem B. Bityuckiy]

> If the "one step" means that you would like to use only one 
> jffs2_decompress/... call than the answer is no.
Ok, thanks for answer, I suspected so. But I was confused by the 
interface function jffs2_free_comprbuf, and slender hope have appeared. :-)

> 
> But I can imagine a solution, where you can call jffs2_compress 
> separatedly for the small buffers, and you can uncompress it using a loop.
> 
> The compression buffer can be something like the following:
>   - original_size_of_small_buffer1
>   - compressed_size_of_small_buffer1
>   - comprtype (return value of jffs2_compress)
>   - compressed data1
>   - original_size_of_small_buffer2
>   - compressed_size_of_small_buffer2
>   - comprtype (return value of jffs2_compress)
>   - compressed data2
>   ...
> 
> I don't know it is OK for you or not.
It would be OK, if I don't have to store the sizes of original buffers 
(original_size_of_small_buffer1, etc) in the node... This is because the 
number of input small buffers isn't fixed...

Thank you.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.