CONFIG_PREEMPT and JFFS2 oops

CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

Hi,

With CONFIG_PREEMPT=y and hammering away on two different JFFS2 
partitions on a NAND flash I get an oops within ~10 seconds.  This is on 
a BeagleBoard xM (rev A2, with NAND).

I've boiled it down to whether CONFIG_PREEMPT (bug happens) or
CONFIG_PREEMPT_VOLUNTARY (bug doesn't happen) is selected.  Of course,
changing that affects a other things like inline spinlocking.  Turning 
on CONFIG_DEBUG_SPINLOCK reveals nothing.


By changing this option, I've made the bug go away in a 2.6.32 and
2.6.37 setup where it previously happened, and I've made it appear in a
2.6.39 setup where it previously didn't happen.


Pointers on what to look at next are appreciated.  (I've posted this on 
the mtd-utils mailing list too.)  More details below.


Thanks,
Orjan


The setup is simply two JFFS2-formatted partitions, and launching a

   while :; do dd if=/dev/zero of=file bs=800 count=1; done

on each of them.  Sometimes the oops trace originates from the garbage 
collector, sometimes the result is a JFFS2 decompress error.


-- 
Orjan Friberg
FlatFrog Laboratories AB

[-- Attachment #2: jffs2_oops --]
[-- Type: text/plain, Size: 4618 bytes --]

[   81.200805] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[   81.217529] pgd = ce13c000
[   81.220855] [00000000] *pgd=8e172031, *pte=00000000, *ppte=00000000
[   81.236480] Internal error: Oops: 17 [#1] PREEMPT
[   81.241210] last sysfs file: /sys/kernel/uevent_seqnum
[   81.246368] Modules linked in: ftdi_sio usbserial
[   81.251129] CPU: 0    Not tainted  (2.6.32 #6)
[   81.255584] PC is at crc32_le+0x6c/0xf4
[   81.259460] LR is at jffs2_write_inode_range+0x2a0/0x420
[   81.264801] pc : [<c0211f28>]    lr : [<c01ae930>]    psr: 20000013
[   81.264801] sp : ce24bcd0  ip : 00000001  fp : ce11f840
[   81.276336] r10: 0000000c  r9 : ce5231d0  r8 : fffffffc
[   81.281585] r7 : 00000002  r6 : 00000000  r5 : c03fcf9c  r4 : 00000000
[   81.288146] r3 : 00000000  r2 : 00000008  r1 : 00000000  r0 : 00000000
[   81.294677] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   81.301849] Control: 10c5387d  Table: 8e13c019  DAC: 00000015
[   81.307617] Process dd (pid: 5270, stack limit = 0xce24a2f0)
[   81.313323] Stack: (0xce24bcd0 to 0xce24c000)
[   81.317687] bcc0:                                     00000000 00000002 00000003 00000000
[   81.325897] bce0: 00000000 c01ae930 ce24bd1c ce24bd18 00000000 00000008 00000000 00000000
[   81.334136] bd00: 00000000 00000002 cdca7000 ce1a8800 00000000 00000000 00000008 00000320
[   81.342346] bd20: 0001326c 00000000 00000320 00000000 ce11f840 ce523208 00000000 c07754e0
[   81.350555] bd40: 00000320 00000000 ce1a8800 c01a8ac4 00000000 00000320 ce24bd74 ffffffff
[   81.358764] bd60: 00000000 00000320 00000000 00000000 00000320 00000000 00000320 00000320
[   81.367004] bd80: 00000000 00000000 00000000 00000320 00000000 00000000 ce5232b0 c0097d1c
[   81.375213] bda0: 00000320 00000320 c07754e0 ce523208 ce24a000 cebf4140 ce5232b0 00001000
[   81.383422] bdc0: 00000000 c03efe38 ce24bf40 00000001 00000000 00000320 ce523208 c07754e0
[   81.391632] bde0: 00000320 00000320 00000000 00000320 ce523208 00000000 00000000 00000000
[   81.399871] be00: 00000000 c009846c 00000000 00000000 ce24bf00 00000320 00000000 00000000
[   81.408081] be20: 00000002 ce24bf00 ce24bf40 ce24beb0 cebf4140 ce5232b0 00000320 00000001
[   81.416290] be40: ce24a000 ce523278 000ad008 c03dd658 22222222 00000320 22222222 ce523278
[   81.424530] be60: ce24bf40 ce24beb0 00000001 00000000 cebf4140 00000000 000ad008 c009851c
[   81.432739] be80: ce24beb0 ce24bf40 00000000 00000000 ce24beb0 cebf4140 ce24bf80 ce24a000
[   81.440948] bea0: 000aad28 c00bf584 00000000 00000000 00020242 ce1ae000 00000000 00000001
[   81.449157] bec0: ffffffff cebf4140 00000000 00000000 00000000 00000000 ce12d6c0 00020241
[   81.457397] bee0: 00000000 00000000 00000200 ce12d6c0 c0077028 ce24bef4 ce24bef4 00000004
[   81.465606] bf00: 00000000 00000000 000aad28 00000300 00000000 00000000 00000320 00100073
[   81.473815] bf20: 000ad000 ce24a000 000ce000 00000000 00000002 ceb450e0 ce4b0618 00000001
[   81.482025] bf40: 000ad008 00000320 cebf4140 000ad008 ce24bf80 00000320 00000320 c00c01c8
[   81.490264] bf60: cebf4140 000ad008 00000000 00000000 cebf4140 00000320 000ad008 c00c036c
[   81.498474] bf80: 00000000 00000000 00000320 00000000 00000320 00000001 000ad008 00000004
[   81.506683] bfa0: c00390c4 c0038f40 00000320 00000001 00000001 000ad008 00000320 000acd34
[   81.514923] bfc0: 00000320 00000001 000ad008 00000004 00000320 000ad008 000aad28 000ad008
[   81.523132] bfe0: 4001e3e0 bece4b60 00010e34 40188abc 60000010 00000001 00000000 00000000
[   81.531372] [<c0211f28>] (crc32_le+0x6c/0xf4) from [<c01ae930>] (jffs2_write_inode_range+0x2a0/0x420)
[   81.540618] [<c01ae930>] (jffs2_write_inode_range+0x2a0/0x420) from [<c01a8ac4>] (jffs2_write_end+0x190/0x2d4)
[   81.550689] [<c01a8ac4>] (jffs2_write_end+0x190/0x2d4) from [<c0097d1c>] (generic_file_buffered_write+0x180/0x264)
[   81.561096] [<c0097d1c>] (generic_file_buffered_write+0x180/0x264) from [<c009846c>] (__generic_file_aio_write+0x468/0x4b0)
[   81.572265] [<c009846c>] (__generic_file_aio_write+0x468/0x4b0) from [<c009851c>] (generic_file_aio_write+0x68/0xc4)
[   81.582855] [<c009851c>] (generic_file_aio_write+0x68/0xc4) from [<c00bf584>] (do_sync_write+0xac/0xfc)
[   81.592285] [<c00bf584>] (do_sync_write+0xac/0xfc) from [<c00c01c8>] (vfs_write+0xac/0x1a4)
[   81.600677] [<c00c01c8>] (vfs_write+0xac/0x1a4) from [<c00c036c>] (sys_write+0x3c/0x68)
[   81.608734] [<c00c036c>] (sys_write+0x3c/0x68) from [<c0038f40>] (ret_fast_syscall+0x0/0x2c)
[   81.617218] Code: e2448004 e3a01000 e1a0c007 ea00000e (e7942001) 
[   82.040069] ---[ end trace 6a60d817de90299e ]---

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/25/2012 09:12 PM, Orjan Friberg wrote:
> I've boiled it down to whether CONFIG_PREEMPT (bug happens) or
> CONFIG_PREEMPT_VOLUNTARY (bug doesn't happen) is selected.

No, I haven't.  The problem disappeared only for

    while :; do dd if=/dev/zero of=file bs=800 count=1; done

That one-liner was boiled down from the following program, which still
oopses instantly:

    #include <stdio.h>
    #include <unistd.h>
    #include <sys/types.h>
    #include <sys/stat.h>
    #include <fcntl.h>

    int main()
    {
      int fd;
      struct stat st;
      char buf[800];

      do {
        unlink("file2");
        fd = open("file1", O_RDWR|O_CREAT|O_TRUNC, 0666);
        stat("file1", &st);
        lseek(fd, 0, SEEK_SET);
        write(fd, buf, 800);
        close(fd);
        rename("file1", "file2");
      } while (1);

      return 0;
    }


(Apologies for spamming.)


-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

Hi

On Wed, 25 Jan 2012, Orjan Friberg wrote:

> With CONFIG_PREEMPT=y and hammering away on two different JFFS2 partitions on
> a NAND flash I get an oops within ~10 seconds.  This is on a BeagleBoard xM
> (rev A2, with NAND).
> 
> I've boiled it down to whether CONFIG_PREEMPT (bug happens) or
> CONFIG_PREEMPT_VOLUNTARY (bug doesn't happen) is selected.  Of course,
> changing that affects a other things like inline spinlocking.  Turning on
> CONFIG_DEBUG_SPINLOCK reveals nothing.
> 
> By changing this option, I've made the bug go away in a 2.6.32 and
> 2.6.37 setup where it previously happened, and I've made it appear in a
> 2.6.39 setup where it previously didn't happen.
> 
> Pointers on what to look at next are appreciated.  (I've posted this on the
> mtd-utils mailing list too.)  More details below.

...

> Sometimes the oops trace originates from the garbage collector, 
> sometimes the result is a JFFS2 decompress error.

The problem is unlikely to be OMAP-specific, given the oops you sent.

Here are some suggestions for debugging:

- Try changing all the spin_lock() calls to spin_lock_irqsave() and all 
  the spin_unlock() calls to spin_unlock_irqrestore() to see if the 
  preemption count is being prematurely decremented

- If your oopses are consistently in the same places, add some debugging 
  to that code to determine which line is actually causing the oops.  
  Either that, or try disassembling the function to see what instruction 
  is causing the problem, and reference that back to the source file.  The 
  latter is actually preferable since it is less likely to cause the 
  problem to mysteriously disappear.  Doing this analysis should provide a 
  good clue as to where to look next.  I personally would be rather 
  suspicious of that

		ri->data_crc = cpu_to_je32(crc32(0, comprbuf, cdatalen));

  in jffs2_write_inode_range().

- Try turning on JFFS2 debugging and seeing if you can reproduce it.  
  The output might provide a clue as to where the problem would be.


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Wed, 25 Jan 2012, Paul Walmsley wrote:

> - Try changing all the spin_lock() calls to spin_lock_irqsave() and all 
>   the spin_unlock() calls to spin_unlock_irqrestore() to see if the 
>   preemption count is being prematurely decremented

Just to clarify, I mean in the jffs2 code, not the entire kernel.


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Wed, 25 Jan 2012, Paul Walmsley wrote:

> On Wed, 25 Jan 2012, Paul Walmsley wrote:
> 
> > - Try changing all the spin_lock() calls to spin_lock_irqsave() and all 
> >   the spin_unlock() calls to spin_unlock_irqrestore() to see if the 
> >   preemption count is being prematurely decremented
> 
> Just to clarify, I mean in the jffs2 code, not the entire kernel.

... and it just occurred to me that this is a waste of time -- that change 
shouldn't affect the preemption count.  So, please ignore that suggestion.  
Sorry about that.


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/25/2012 10:02 PM, Orjan Friberg wrote:
> That one-liner was boiled down from the following program, which still
> oopses instantly:

The C program seems to work fine with CONFIG_PREEMPT_NONE=y.

If that is indeed the problem I guess it's reasonable that it worked
better with PREEMPT_VOLUNTARY than PREEMPT because there are fewer
preemtion points.

-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/25/2012 10:18 PM, Paul Walmsley wrote:
> - If your oopses are consistently in the same places, add some debugging
>    to that code to determine which line is actually causing the oops.

(CC:d linux-mtd.)

They are semi-consistent I'd say.  The oops trace I posted is by far the 
most common.

>    problem to mysteriously disappear.  Doing this analysis should provide a
>    good clue as to where to look next.  I personally would be rather
>    suspicious of that
>
> 		ri->data_crc = cpu_to_je32(crc32(0, comprbuf, cdatalen));
>
>    in jffs2_write_inode_range().

That is indeed the place where crc32 is called from .  I'll see it I can 
track the use of comprbuf.

> - Try turning on JFFS2 debugging and seeing if you can reproduce it.
>    The output might provide a clue as to where the problem would be.

Here are two examples (immediately preceding the oops):

jffs2_reserve_space(): Requested 0x30 bytes
jffs2_reserve_space(): alloc sem got
[JFFS2 DBG] (1189) jffs2_do_reserve_space: minsize=48 , jeb->free=46852 
,summary->size=16586 , sumsize=29
jffs2_do_reserve_space(): Giving 0x75f4 bytes at 0x3d48fc
jffs2_write_dirent(ino #1, name at *0xdea7b93c "file1"->ino #111, 
name_crc 0x58c597f8)


jffs2_write_begin()
jffs2_read_inode_range: ino #12, range 0x00000000-0x00001000
Filling non-frag hole from 0-4096
end write_begin(). pg->flags 9
jffs2_write_end(): ino #12, page at 0x0, range 0-800, flags d
jffs2_write_inode_range(): Ino #12, ofs 0x0, len 0x320
jffs2_reserve_space(): Requested 0xc4 bytes
jffs2_reserve_space(): alloc sem got
[JFFS2 DBG] (1454) jffs2_do_reserve_space: minsize=196 , 
jeb->free=123148 ,summary->size=1567 , sumsize=18
jffs2_do_reserve_space(): Giving 0x1dab0 bytes at 0xf941ef4
calling deflate with avail_in 788, avail_out 788
deflate returned with avail_in 0, avail_out 428, total_in 788, total_out 360
calling deflate with avail_in 12, avail_out 428
deflate returned with avail_in 0, avail_out 414, total_in 800, total_out 374
zlib compressed 800 bytes into 380


I'll take a look at what jffs2_do_reserve_space is up to.


Thanks.

-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/25/2012 10:18 PM, Paul Walmsley wrote:
> - If your oopses are consistently in the same places, add some debugging
>    to that code to determine which line is actually causing the oops.

(CC:d linux-mtd.)

They are semi-consistent I'd say.  The oops trace I posted is by far the 
most common.

>    problem to mysteriously disappear.  Doing this analysis should provide a
>    good clue as to where to look next.  I personally would be rather
>    suspicious of that
>
> 		ri->data_crc = cpu_to_je32(crc32(0, comprbuf, cdatalen));
>
>    in jffs2_write_inode_range().

That is indeed the place where crc32 is called from .  I'll see it I can 
track the use of comprbuf.

> - Try turning on JFFS2 debugging and seeing if you can reproduce it.
>    The output might provide a clue as to where the problem would be.

Here are two examples (immediately preceding the oops):

jffs2_reserve_space(): Requested 0x30 bytes
jffs2_reserve_space(): alloc sem got
[JFFS2 DBG] (1189) jffs2_do_reserve_space: minsize=48 , jeb->free=46852 
,summary->size=16586 , sumsize=29
jffs2_do_reserve_space(): Giving 0x75f4 bytes at 0x3d48fc
jffs2_write_dirent(ino #1, name at *0xdea7b93c "file1"->ino #111, 
name_crc 0x58c597f8)


jffs2_write_begin()
jffs2_read_inode_range: ino #12, range 0x00000000-0x00001000
Filling non-frag hole from 0-4096
end write_begin(). pg->flags 9
jffs2_write_end(): ino #12, page at 0x0, range 0-800, flags d
jffs2_write_inode_range(): Ino #12, ofs 0x0, len 0x320
jffs2_reserve_space(): Requested 0xc4 bytes
jffs2_reserve_space(): alloc sem got
[JFFS2 DBG] (1454) jffs2_do_reserve_space: minsize=196 , 
jeb->free=123148 ,summary->size=1567 , sumsize=18
jffs2_do_reserve_space(): Giving 0x1dab0 bytes at 0xf941ef4
calling deflate with avail_in 788, avail_out 788
deflate returned with avail_in 0, avail_out 428, total_in 788, total_out 360
calling deflate with avail_in 12, avail_out 428
deflate returned with avail_in 0, avail_out 414, total_in 800, total_out 374
zlib compressed 800 bytes into 380


I'll take a look at what jffs2_do_reserve_space is up to.


Thanks.

-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

>
> On 01/25/2012 10:18 PM, Paul Walmsley wrote:
> > - If your oopses are consistently in the same places, add some debugging
> >    to that code to determine which line is actually causing the oops.
>
> (CC:d linux-mtd.)
>
> They are semi-consistent I'd say.  The oops trace I posted is by far the
> most common.
>
> >    problem to mysteriously disappear.  Doing this analysis should provide a
> >    good clue as to where to look next.  I personally would be rather
> >    suspicious of that
> >
> >       ri->data_crc = cpu_to_je32(crc32(0, comprbuf, cdatalen));
> >
> >    in jffs2_write_inode_range().
>
> That is indeed the place where crc32 is called from .  I'll see it I can
> track the use of comprbuf.
>
> > - Try turning on JFFS2 debugging and seeing if you can reproduce it.
> >    The output might provide a clue as to where the problem would be.
>

I would also turn off SUMMARY in JFFS2, I tried it long ago and it didn't make
much of a difference.

 Jocke


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

>
> On 01/25/2012 10:18 PM, Paul Walmsley wrote:
> > - If your oopses are consistently in the same places, add some debugging
> >    to that code to determine which line is actually causing the oops.
>
> (CC:d linux-mtd.)
>
> They are semi-consistent I'd say.  The oops trace I posted is by far the
> most common.
>
> >    problem to mysteriously disappear.  Doing this analysis should provide a
> >    good clue as to where to look next.  I personally would be rather
> >    suspicious of that
> >
> >       ri->data_crc = cpu_to_je32(crc32(0, comprbuf, cdatalen));
> >
> >    in jffs2_write_inode_range().
>
> That is indeed the place where crc32 is called from .  I'll see it I can
> track the use of comprbuf.
>
> > - Try turning on JFFS2 debugging and seeing if you can reproduce it.
> >    The output might provide a clue as to where the problem would be.
>

I would also turn off SUMMARY in JFFS2, I tried it long ago and it didn't make
much of a difference.

 Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/26/2012 11:15 AM, Orjan Friberg wrote:
>>     problem to mysteriously disappear.  Doing this analysis should provide a
>>     good clue as to where to look next.  I personally would be rather
>>     suspicious of that
>>
>> 		ri->data_crc = cpu_to_je32(crc32(0, comprbuf, cdatalen));
>>
>>     in jffs2_write_inode_range().
>
> That is indeed the place where crc32 is called from .  I'll see it I can
> track the use of comprbuf.

Ok, so comprbuf comes from jffs2_compress and becomes NULL for some 
reason (hence the oops).

Initially I had CMODE_FAVOUR_LZO.  With that, things only worked with 
PREEMPT_NONE.  However, when changing to CMODE_PRIORITY or CMODE_NONE 
things do seem to work *with* PREEMPT.

For what it's worth (with PREEMPT on):

CMODE_FAVOUR_LZO with LZO disabled oopses.
CMODE_FAVOUR_LZO with only ZLIB enabled oopses.
CMODE_FAVOUR_LZO with ZLIB/LZO/RTIME/RUBIN disabled does not oops.

Thus, the bug seems to be in the *selection* of compression algorithm 
(when there is at least one algoritm in the list), rather than in the 
specific compression algorithms themselves.


-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Orjan Friberg wrote:

> Ok, so comprbuf comes from jffs2_compress and becomes NULL for some reason
> (hence the oops).
> 
> Initially I had CMODE_FAVOUR_LZO.  With that, things only worked with
> PREEMPT_NONE.  However, when changing to CMODE_PRIORITY or CMODE_NONE things
> do seem to work *with* PREEMPT.
> 
> For what it's worth (with PREEMPT on):
> 
> CMODE_FAVOUR_LZO with LZO disabled oopses.
> CMODE_FAVOUR_LZO with only ZLIB enabled oopses.
> CMODE_FAVOUR_LZO with ZLIB/LZO/RTIME/RUBIN disabled does not oops.
> 
> Thus, the bug seems to be in the *selection* of compression algorithm (when
> there is at least one algoritm in the list), rather than in the specific
> compression algorithms themselves.

The locking in jffs2_compress() isn't right.  this->compr_buf could be 
freed or reassigned while the compressor is busy using it.  If I'm reading 
the code correctly, this problem could also cause data and metadata 
corruption.

Want to give the following untested patch a try?  Of course, it could 
destroy everything.  So it shouldn't be used on anything important.  But 
it might fix the problem you're seeing.  The patch will need significant 
testing and review by JFFS2 experts before getting merged.  


- Paul


From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH] fs: jffs2: compression: fix some (but not all) races in
 jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregister while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   66 ++++++++++++++++++++++-------------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..91feb31 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -151,8 +154,8 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
 	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,61 +171,48 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if (((!best_dlen) || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				best = this;
 			}
 		}
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = tmp_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
 			ret = best->compr;
 			*cpage_out = output_buf;
+		} else {
+			kfree(tmp_buf);
 		}
 		spin_unlock(&jffs2_compressor_list_lock);
 		break;
@@ -302,8 +292,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Orjan Friberg wrote:

> Ok, so comprbuf comes from jffs2_compress and becomes NULL for some reason
> (hence the oops).
> 
> Initially I had CMODE_FAVOUR_LZO.  With that, things only worked with
> PREEMPT_NONE.  However, when changing to CMODE_PRIORITY or CMODE_NONE things
> do seem to work *with* PREEMPT.
> 
> For what it's worth (with PREEMPT on):
> 
> CMODE_FAVOUR_LZO with LZO disabled oopses.
> CMODE_FAVOUR_LZO with only ZLIB enabled oopses.
> CMODE_FAVOUR_LZO with ZLIB/LZO/RTIME/RUBIN disabled does not oops.
> 
> Thus, the bug seems to be in the *selection* of compression algorithm (when
> there is at least one algoritm in the list), rather than in the specific
> compression algorithms themselves.

The locking in jffs2_compress() isn't right.  this->compr_buf could be 
freed or reassigned while the compressor is busy using it.  If I'm reading 
the code correctly, this problem could also cause data and metadata 
corruption.

Want to give the following untested patch a try?  Of course, it could 
destroy everything.  So it shouldn't be used on anything important.  But 
it might fix the problem you're seeing.  The patch will need significant 
testing and review by JFFS2 experts before getting merged.  


- Paul


From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH] fs: jffs2: compression: fix some (but not all) races in
 jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregister while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   66 ++++++++++++++++++++++-------------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..91feb31 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -151,8 +154,8 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
 	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,61 +171,48 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if (((!best_dlen) || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				best = this;
 			}
 		}
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = tmp_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
 			ret = best->compr;
 			*cpage_out = output_buf;
+		} else {
+			kfree(tmp_buf);
 		}
 		spin_unlock(&jffs2_compressor_list_lock);
 		break;
@@ -302,8 +292,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

> From: Paul Walmsley <paul@pwsan.com>
> To: Orjan Friberg <of@flatfrog.com>
> Cc: David Woodhouse <dwmw2@infradead.org>, Richard Purdie <rpurdie@openedhand.com>, "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>, "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>
> Date: 2012/01/26 17:14
> Subject: Re: CONFIG_PREEMPT and JFFS2 oops
> Sent by: linux-mtd-bounces@lists.infradead.org
>
> On Thu, 26 Jan 2012, Orjan Friberg wrote:
>
> > Ok, so comprbuf comes from jffs2_compress and becomes NULL for some reason
> > (hence the oops).
> >
> > Initially I had CMODE_FAVOUR_LZO.  With that, things only worked with
> > PREEMPT_NONE.  However, when changing to CMODE_PRIORITY or CMODE_NONE things
> > do seem to work *with* PREEMPT.
> >
> > For what it's worth (with PREEMPT on):
> >
> > CMODE_FAVOUR_LZO with LZO disabled oopses.
> > CMODE_FAVOUR_LZO with only ZLIB enabled oopses.
> > CMODE_FAVOUR_LZO with ZLIB/LZO/RTIME/RUBIN disabled does not oops.
> >
> > Thus, the bug seems to be in the *selection* of compression algorithm (when
> > there is at least one algoritm in the list), rather than in the specific
> > compression algorithms themselves.
>
> The locking in jffs2_compress() isn't right.  this->compr_buf could be
> freed or reassigned while the compressor is busy using it.  If I'm reading
> the code correctly, this problem could also cause data and metadata
> corruption.
>
> Want to give the following untested patch a try?  Of course, it could
> destroy everything.  So it shouldn't be used on anything important.  But
> it might fix the problem you're seeing.  The patch will need significant
> testing and review by JFFS2 experts before getting merged.
>
>
> - Paul
>
>
> From: Paul Walmsley <paul@pwsan.com>
> Date: Thu, 26 Jan 2012 08:12:09 -0700
> Subject: [PATCH] fs: jffs2: compression: fix some (but not all) races in
>  jffs2_compress()
>
> There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
> or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
> filesystems are in use.  The compressor buffer is shared among all
> users of the compressor, and the buffer is freed and allocated without
> holding any lock.  This could result in NULL pointer dereferences, or,
> worse, corrupted data.
>
> There doesn't appear to be any point in having a compression buffer
> shared by all users of the compressor.  So remove this, and instead
> use a buffer that is local to the jffs2_compress() call.  This
> simplifies the locking in this function considerably.
>
> There's at least one race left in this function, between it and
> jffs2_unregister_compressor().  That's left for someone else to fix.
> Until then, it is suggested that compressors should not be registered
> or unregister while any JFFS2 filesystems are mounted.
>
> This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
> FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
> thoroughly and the code has been reviewed by JFFS2 experts.

I think I found one bug by looking at the patch. You need 2 buffers, one
that holds the latest compressed data and one working buffer.

  Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

> From: Paul Walmsley <paul@pwsan.com>
> To: Orjan Friberg <of@flatfrog.com>
> Cc: David Woodhouse <dwmw2@infradead.org>, Richard Purdie <rpurdie@openedhand.com>, "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>, "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>
> Date: 2012/01/26 17:14
> Subject: Re: CONFIG_PREEMPT and JFFS2 oops
> Sent by: linux-mtd-bounces@lists.infradead.org
>
> On Thu, 26 Jan 2012, Orjan Friberg wrote:
>
> > Ok, so comprbuf comes from jffs2_compress and becomes NULL for some reason
> > (hence the oops).
> >
> > Initially I had CMODE_FAVOUR_LZO.  With that, things only worked with
> > PREEMPT_NONE.  However, when changing to CMODE_PRIORITY or CMODE_NONE things
> > do seem to work *with* PREEMPT.
> >
> > For what it's worth (with PREEMPT on):
> >
> > CMODE_FAVOUR_LZO with LZO disabled oopses.
> > CMODE_FAVOUR_LZO with only ZLIB enabled oopses.
> > CMODE_FAVOUR_LZO with ZLIB/LZO/RTIME/RUBIN disabled does not oops.
> >
> > Thus, the bug seems to be in the *selection* of compression algorithm (when
> > there is at least one algoritm in the list), rather than in the specific
> > compression algorithms themselves.
>
> The locking in jffs2_compress() isn't right.  this->compr_buf could be
> freed or reassigned while the compressor is busy using it.  If I'm reading
> the code correctly, this problem could also cause data and metadata
> corruption.
>
> Want to give the following untested patch a try?  Of course, it could
> destroy everything.  So it shouldn't be used on anything important.  But
> it might fix the problem you're seeing.  The patch will need significant
> testing and review by JFFS2 experts before getting merged.
>
>
> - Paul
>
>
> From: Paul Walmsley <paul@pwsan.com>
> Date: Thu, 26 Jan 2012 08:12:09 -0700
> Subject: [PATCH] fs: jffs2: compression: fix some (but not all) races in
>  jffs2_compress()
>
> There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
> or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
> filesystems are in use.  The compressor buffer is shared among all
> users of the compressor, and the buffer is freed and allocated without
> holding any lock.  This could result in NULL pointer dereferences, or,
> worse, corrupted data.
>
> There doesn't appear to be any point in having a compression buffer
> shared by all users of the compressor.  So remove this, and instead
> use a buffer that is local to the jffs2_compress() call.  This
> simplifies the locking in this function considerably.
>
> There's at least one race left in this function, between it and
> jffs2_unregister_compressor().  That's left for someone else to fix.
> Until then, it is suggested that compressors should not be registered
> or unregister while any JFFS2 filesystems are mounted.
>
> This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
> FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
> thoroughly and the code has been reviewed by JFFS2 experts.

I think I found one bug by looking at the patch. You need 2 buffers, one
that holds the latest compressed data and one working buffer.

  Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> I think I found one bug by looking at the patch. You need 2 buffers, one
> that holds the latest compressed data and one working buffer.

Thanks, I think you're right.  Here's an updated patch.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH v2] fs: jffs2: compression: fix some (but not all) races 
in jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregistered while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com> and Joakim Tjernlund <joakim.tjernlund@transmode.se>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   73 ++++++++++++++++++++++++-----------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..15c300a 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -150,9 +153,9 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int ret = JFFS2_COMPR_NONE;
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
-	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
+	unsigned char *output_buf = NULL, *tmp_buf, *best_buf;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,56 +171,48 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
+		best_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!best_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if ((!best_dlen || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				best_buf = tmp_buf;
+				best = this;
 			}
 		}
+		kfree(tmp_buf);
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = best_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
@@ -302,8 +297,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> I think I found one bug by looking at the patch. You need 2 buffers, one
> that holds the latest compressed data and one working buffer.

Thanks, I think you're right.  Here's an updated patch.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH v2] fs: jffs2: compression: fix some (but not all) races 
in jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregistered while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com> and Joakim Tjernlund <joakim.tjernlund@transmode.se>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   73 ++++++++++++++++++++++++-----------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..15c300a 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -150,9 +153,9 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int ret = JFFS2_COMPR_NONE;
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
-	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
+	unsigned char *output_buf = NULL, *tmp_buf, *best_buf;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,56 +171,48 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
+		best_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!best_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if ((!best_dlen || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				best_buf = tmp_buf;
+				best = this;
 			}
 		}
+		kfree(tmp_buf);
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = best_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
@@ -302,8 +297,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

Paul,

Your patch works fine in that it doesn't oops, and I'm not seeing any 
BUGs from CONFIG_DEBUG_SPINLOCK.  I haven't verified *anything else* 
(performance etc).

We've had some discussions on the linux-mtd list during the day, 
starting at 
http://lists.infradead.org/pipermail/linux-mtd/2012-January/039442.html 
if you're interested (though that discussion didn't result in a patch).

Thanks,
Orjan


-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

Paul,

Your patch works fine in that it doesn't oops, and I'm not seeing any 
BUGs from CONFIG_DEBUG_SPINLOCK.  I haven't verified *anything else* 
(performance etc).

We've had some discussions on the linux-mtd list during the day, 
starting at 
http://lists.infradead.org/pipermail/linux-mtd/2012-January/039442.html 
if you're interested (though that discussion didn't result in a patch).

Thanks,
Orjan


-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Paul Walmsley wrote:

> On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
> 
> > I think I found one bug by looking at the patch. You need 2 buffers, one
> > that holds the latest compressed data and one working buffer.
> 
> Thanks, I think you're right.  Here's an updated patch.

... which has another minor bug: it leaks memory if the first allocation 
fails.  Here's a version with that fixed.


- Paul

>From 9ed208134d1e1fec834bcb9a6cad556f54178889 Mon Sep 17 00:00:00 2001
From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH] fs: jffs2: compression: fix some (but not all) races in
 jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregistered while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com> and Joakim Tjernlund <joakim.tjernlund@transmode.se>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   74 ++++++++++++++++++++++++-----------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..377634b 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -150,9 +153,9 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int ret = JFFS2_COMPR_NONE;
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
-	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
+	unsigned char *output_buf = NULL, *tmp_buf, *best_buf;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,56 +171,49 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
+		best_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!best_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			kfree(tmp_buf);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if ((!best_dlen || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				best_buf = tmp_buf;
+				best = this;
 			}
 		}
+		kfree(tmp_buf);
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = best_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
@@ -302,8 +298,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Paul Walmsley wrote:

> On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
> 
> > I think I found one bug by looking at the patch. You need 2 buffers, one
> > that holds the latest compressed data and one working buffer.
> 
> Thanks, I think you're right.  Here's an updated patch.

... which has another minor bug: it leaks memory if the first allocation 
fails.  Here's a version with that fixed.


- Paul

>From 9ed208134d1e1fec834bcb9a6cad556f54178889 Mon Sep 17 00:00:00 2001
From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH] fs: jffs2: compression: fix some (but not all) races in
 jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregistered while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com> and Joakim Tjernlund <joakim.tjernlund@transmode.se>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   74 ++++++++++++++++++++++++-----------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..377634b 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -150,9 +153,9 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int ret = JFFS2_COMPR_NONE;
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
-	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
+	unsigned char *output_buf = NULL, *tmp_buf, *best_buf;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,56 +171,49 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
+		best_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!best_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			kfree(tmp_buf);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if ((!best_dlen || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				best_buf = tmp_buf;
+				best = this;
 			}
 		}
+		kfree(tmp_buf);
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = best_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
@@ -302,8 +298,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Orjan Friberg wrote:

> Your patch works fine in that it doesn't oops, and I'm not seeing any BUGs
> from CONFIG_DEBUG_SPINLOCK.  I haven't verified *anything else* (performance
> etc).
> 
> We've had some discussions on the linux-mtd list during the day, starting at
> http://lists.infradead.org/pipermail/linux-mtd/2012-January/039442.html if
> you're interested (though that discussion didn't result in a patch).

Thanks for the pointer, I'm not subscribed.  That's good analysis.

As Joakim pointed out, that first patch had a pretty serious bug itself.  
So perhaps if you have the time, it might be worth trying this updated 
version:

http://marc.info/?l=linux-omap&m=132759610628997&w=2


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Orjan Friberg wrote:

> Your patch works fine in that it doesn't oops, and I'm not seeing any BUGs
> from CONFIG_DEBUG_SPINLOCK.  I haven't verified *anything else* (performance
> etc).
> 
> We've had some discussions on the linux-mtd list during the day, starting at
> http://lists.infradead.org/pipermail/linux-mtd/2012-January/039442.html if
> you're interested (though that discussion didn't result in a patch).

Thanks for the pointer, I'm not subscribed.  That's good analysis.

As Joakim pointed out, that first patch had a pretty serious bug itself.  
So perhaps if you have the time, it might be worth trying this updated 
version:

http://marc.info/?l=linux-omap&m=132759610628997&w=2


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

Paul Walmsley <paul@pwsan.com> wrote on 2012/01/26 17:38:46:
>
> On Thu, 26 Jan 2012, Paul Walmsley wrote:
>
> > On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
> >
> > > I think I found one bug by looking at the patch. You need 2 buffers, one
> > > that holds the latest compressed data and one working buffer.
> >
> > Thanks, I think you're right.  Here's an updated patch.
>
> ... which has another minor bug: it leaks memory if the first allocation
> fails.  Here's a version with that fixed.

Still leeks:
+		 		 		 		 best_slen = tdl;
+		 		 		 		 best_buf = tmp_buf;
+		 		 		 		 best = this;

You just throw away best_buf here, don't you?

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

Paul Walmsley <paul@pwsan.com> wrote on 2012/01/26 17:38:46:
>
> On Thu, 26 Jan 2012, Paul Walmsley wrote:
>
> > On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
> >
> > > I think I found one bug by looking at the patch. You need 2 buffers, one
> > > that holds the latest compressed data and one working buffer.
> >
> > Thanks, I think you're right.  Here's an updated patch.
>
> ... which has another minor bug: it leaks memory if the first allocation
> fails.  Here's a version with that fixed.

Still leeks:
+		 		 		 		 best_slen = tdl;
+		 		 		 		 best_buf = tmp_buf;
+		 		 		 		 best = this;

You just throw away best_buf here, don't you?

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> Still leeks:
> +		 		 		 		 best_slen = tdl;
> +		 		 		 		 best_buf = tmp_buf;
> +		 		 		 		 best = this;
> 
> You just throw away best_buf here, don't you?

You're right.  It's even worse than that.  best_buf will contain the data 
from the last compressor used.  And it will be prematurely freed.  Here's 
a fixed version.

Time for breakfast.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH v4] fs: jffs2: compression: fix some (but not all) races 
in jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregistered while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com> and Joakim Tjernlund <joakim.tjernlund@transmode.se>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   74 ++++++++++++++++++++++++-----------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..77e5091 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -150,9 +153,9 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int ret = JFFS2_COMPR_NONE;
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
-	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
+	unsigned char *output_buf = NULL, *tmp_buf, *best_buf;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,56 +171,49 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
+		best_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!best_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			kfree(tmp_buf);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if ((!best_dlen || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				memcpy(best_buf, tmp_buf, tcdl);
+				best = this;
 			}
 		}
+		kfree(tmp_buf);
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = best_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
@@ -302,8 +298,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> Still leeks:
> +		 		 		 		 best_slen = tdl;
> +		 		 		 		 best_buf = tmp_buf;
> +		 		 		 		 best = this;
> 
> You just throw away best_buf here, don't you?

You're right.  It's even worse than that.  best_buf will contain the data 
from the last compressor used.  And it will be prematurely freed.  Here's 
a fixed version.

Time for breakfast.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 26 Jan 2012 08:12:09 -0700
Subject: [PATCH v4] fs: jffs2: compression: fix some (but not all) races 
in jffs2_compress()

There is a nasty race in jffs2_compress() when JFFS2_COMPR_MODE_SIZE
or JFFS2_COMPR_MODE_FAVOURLZO is selected and multiple jffs2
filesystems are in use.  The compressor buffer is shared among all
users of the compressor, and the buffer is freed and allocated without
holding any lock.  This could result in NULL pointer dereferences, or,
worse, corrupted data.

There doesn't appear to be any point in having a compression buffer
shared by all users of the compressor.  So remove this, and instead
use a buffer that is local to the jffs2_compress() call.  This
simplifies the locking in this function considerably.

There's at least one race left in this function, between it and
jffs2_unregister_compressor().  That's left for someone else to fix.
Until then, it is suggested that compressors should not be registered
or unregistered while any JFFS2 filesystems are mounted.

This patch is COMPLETELY UNTESTED.  It could easily DESTROY THE
FILESYSTEM and CORRUPT DATA, so don't use it unless it's been tested
thoroughly and the code has been reviewed by JFFS2 experts.

This patch was developed in collaboration with Orjan Friberg
<of@flatfrog.com> and Joakim Tjernlund <joakim.tjernlund@transmode.se>.

Not-signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Orjan Friberg <of@flatfrog.com>
Cc: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Cc: Richard Purdie <rpurdie@openedhand.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/compr.c |   74 ++++++++++++++++++++++++-----------------------------
 fs/jffs2/compr.h |    2 -
 2 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 5b6c9d1..77e5091 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -142,6 +142,9 @@ static int jffs2_selected_compress(u8 compr, unsigned char *data_in,
  * If the cdata buffer isn't large enough to hold all the uncompressed data,
  * jffs2_compress should compress as much as will fit, and should set
  * *datalen accordingly to show the amount of data which were compressed.
+ *
+ * Caller must call jffs2_free_comprbuf() after it is done with
+ * @cpage_out.
  */
 uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 			unsigned char *data_in, unsigned char **cpage_out,
@@ -150,9 +153,9 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 	int ret = JFFS2_COMPR_NONE;
 	int mode, compr_ret;
 	struct jffs2_compressor *this, *best=NULL;
-	unsigned char *output_buf = NULL, *tmp_buf;
-	uint32_t orig_slen, orig_dlen;
+	unsigned char *output_buf = NULL, *tmp_buf, *best_buf;
 	uint32_t best_slen=0, best_dlen=0;
+	uint32_t tdl, tcdl;
 
 	if (c->mount_opts.override_compr)
 		mode = c->mount_opts.compr;
@@ -168,56 +171,49 @@ uint16_t jffs2_compress(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
 		break;
 	case JFFS2_COMPR_MODE_SIZE:
 	case JFFS2_COMPR_MODE_FAVOURLZO:
-		orig_slen = *datalen;
-		orig_dlen = *cdatalen;
+		tmp_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!tmp_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			break;
+		}
+		best_buf = kmalloc(*datalen, GFP_KERNEL);
+		if (!best_buf) {
+			pr_warn("JFFS2: No memory for compressor allocation. (%d bytes)\n", *datalen);
+			kfree(tmp_buf);
+			break;
+		}
 		spin_lock(&jffs2_compressor_list_lock);
 		list_for_each_entry(this, &jffs2_compressor_list, list) {
 			/* Skip decompress-only backwards-compatibility and disabled modules */
 			if ((!this->compress)||(this->disabled))
 				continue;
-			/* Allocating memory for output buffer if necessary */
-			if ((this->compr_buf_size < orig_slen) && (this->compr_buf)) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				kfree(this->compr_buf);
-				spin_lock(&jffs2_compressor_list_lock);
-				this->compr_buf_size=0;
-				this->compr_buf=NULL;
-			}
-			if (!this->compr_buf) {
-				spin_unlock(&jffs2_compressor_list_lock);
-				tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
-				spin_lock(&jffs2_compressor_list_lock);
-				if (!tmp_buf) {
-					printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
-					continue;
-				}
-				else {
-					this->compr_buf = tmp_buf;
-					this->compr_buf_size = orig_slen;
-				}
-			}
 			this->usecount++;
+			/*
+			 * XXX This spin_unlock() will cause a race with
+			 * anything that modifies jffs2_compressor_list,
+			 * particularly jffs2_unregister_compressor()
+			 */
 			spin_unlock(&jffs2_compressor_list_lock);
-			*datalen  = orig_slen;
-			*cdatalen = orig_dlen;
-			compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
+			tdl = *datalen;
+			tcdl = *cdatalen;
+			compr_ret = this->compress(data_in, tmp_buf, &tdl, &tcdl);
 			spin_lock(&jffs2_compressor_list_lock);
 			this->usecount--;
-			if (!compr_ret) {
-				if (((!best_dlen) || jffs2_is_best_compression(this, best, *cdatalen, best_dlen))
-						&& (*cdatalen < *datalen)) {
-					best_dlen = *cdatalen;
-					best_slen = *datalen;
-					best = this;
-				}
+			if (compr_ret)
+				continue;
+			if ((!best_dlen || jffs2_is_best_compression(this, best, tcdl, best_dlen))
+			    && (tcdl < tdl)) {
+				best_dlen = tcdl;
+				best_slen = tdl;
+				memcpy(best_buf, tmp_buf, tcdl);
+				best = this;
 			}
 		}
+		kfree(tmp_buf);
 		if (best_dlen) {
 			*cdatalen = best_dlen;
 			*datalen  = best_slen;
-			output_buf = best->compr_buf;
-			best->compr_buf = NULL;
-			best->compr_buf_size = 0;
+			output_buf = best_buf;
 			best->stat_compr_blocks++;
 			best->stat_compr_orig_size += best_slen;
 			best->stat_compr_new_size  += best_dlen;
@@ -302,8 +298,6 @@ int jffs2_register_compressor(struct jffs2_compressor *comp)
 		printk(KERN_WARNING "NULL compressor name at registering JFFS2 compressor. Failed.\n");
 		return -1;
 	}
-	comp->compr_buf_size=0;
-	comp->compr_buf=NULL;
 	comp->usecount=0;
 	comp->stat_compr_orig_size=0;
 	comp->stat_compr_new_size=0;
diff --git a/fs/jffs2/compr.h b/fs/jffs2/compr.h
index 5e91d57..fef088a 100644
--- a/fs/jffs2/compr.h
+++ b/fs/jffs2/compr.h
@@ -56,8 +56,6 @@ struct jffs2_compressor {
 			  uint32_t cdatalen, uint32_t datalen);
 	int usecount;
 	int disabled;		/* if set the compressor won't compress */
-	unsigned char *compr_buf;	/* used by size compr. mode */
-	uint32_t compr_buf_size;	/* used by size compr. mode */
 	uint32_t stat_compr_orig_size;
 	uint32_t stat_compr_new_size;
 	uint32_t stat_compr_blocks;
-- 
1.7.8.3

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

> From: Paul Walmsley <paul@pwsan.com>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Cc: "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>, Richard Purdie <rpurdie@openedhand.com>, "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>, David Woodhouse <dwmw2@infradead.org>, Orjan Friberg <of@flatfrog.com>
> Date: 2012/01/26 18:00
> Subject: Re: CONFIG_PREEMPT and JFFS2 oops
> Sent by: linux-mtd-bounces@lists.infradead.org
>
> On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
>
> > Still leeks:
> > +                            best_slen = tdl;
> > +                            best_buf = tmp_buf;
> > +                            best = this;
> >
> > You just throw away best_buf here, don't you?
>
> You're right.  It's even worse than that.  best_buf will contain the data
> from the last compressor used.  And it will be prematurely freed.  Here's
> a fixed version.
>
> Time for breakfast.

hmm, that memcpy is costly. Just swap tmp_buf and best_buf instead?

Time to go home :)

 Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

> From: Paul Walmsley <paul@pwsan.com>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Cc: "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>, Richard Purdie <rpurdie@openedhand.com>, "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>, David Woodhouse <dwmw2@infradead.org>, Orjan Friberg <of@flatfrog.com>
> Date: 2012/01/26 18:00
> Subject: Re: CONFIG_PREEMPT and JFFS2 oops
> Sent by: linux-mtd-bounces@lists.infradead.org
>
> On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
>
> > Still leeks:
> > +                            best_slen = tdl;
> > +                            best_buf = tmp_buf;
> > +                            best = this;
> >
> > You just throw away best_buf here, don't you?
>
> You're right.  It's even worse than that.  best_buf will contain the data
> from the last compressor used.  And it will be prematurely freed.  Here's
> a fixed version.
>
> Time for breakfast.

hmm, that memcpy is costly. Just swap tmp_buf and best_buf instead?

Time to go home :)

 Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/26/2012 05:57 PM, Paul Walmsley wrote:
>>
>> You just throw away best_buf here, don't you?
>
> You're right.  It's even worse than that.  best_buf will contain the data
> from the last compressor used.  And it will be prematurely freed.  Here's
> a fixed version.

I've tested this version for a while now with the same result as before.

No oopses, no spinlock violations.  I copied a 2MB file from the SD/MMC 
partition to the two JFFS2 partitions and md5summ'ed it a bunch of 
times.  After that I unmounted and remounted both partitions.

I do see a steady memory usage increase when doing continuous testing, 
but whether that's normal I don't know.  I see at least some of it being 
reclaimed when unmounting the JFFS2 partitions (grep jffs2 /proc/slabinfo).

-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Orjan Friberg]

On 01/26/2012 05:57 PM, Paul Walmsley wrote:
>>
>> You just throw away best_buf here, don't you?
>
> You're right.  It's even worse than that.  best_buf will contain the data
> from the last compressor used.  And it will be prematurely freed.  Here's
> a fixed version.

I've tested this version for a while now with the same result as before.

No oopses, no spinlock violations.  I copied a 2MB file from the SD/MMC 
partition to the two JFFS2 partitions and md5summ'ed it a bunch of 
times.  After that I unmounted and remounted both partitions.

I do see a steady memory usage increase when doing continuous testing, 
but whether that's normal I don't know.  I see at least some of it being 
reclaimed when unmounting the JFFS2 partitions (grep jffs2 /proc/slabinfo).

-- 
Orjan Friberg
FlatFrog Laboratories AB

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

> >
> > On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
> >
> > > Still leeks:
> > > +                            best_slen = tdl;
> > > +                            best_buf = tmp_buf;
> > > +                            best = this;
> > >
> > > You just throw away best_buf here, don't you?
> >
> > You're right.  It's even worse than that.  best_buf will contain the data
> > from the last compressor used.  And it will be prematurely freed.  Here's
> > a fixed version.
> >
> > Time for breakfast.
>
> hmm, that memcpy is costly. Just swap tmp_buf and best_buf instead?
>
> Time to go home :)

.. and you should delay the second allocation to when it is needed. After swapping ptrs, test if
tmp_buf is NULL and kmalloc if so.

 Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

> >
> > On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
> >
> > > Still leeks:
> > > +                            best_slen = tdl;
> > > +                            best_buf = tmp_buf;
> > > +                            best = this;
> > >
> > > You just throw away best_buf here, don't you?
> >
> > You're right.  It's even worse than that.  best_buf will contain the data
> > from the last compressor used.  And it will be prematurely freed.  Here's
> > a fixed version.
> >
> > Time for breakfast.
>
> hmm, that memcpy is costly. Just swap tmp_buf and best_buf instead?
>
> Time to go home :)

.. and you should delay the second allocation to when it is needed. After swapping ptrs, test if
tmp_buf is NULL and kmalloc if so.

 Jocke

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> hmm, that memcpy is costly. Just swap tmp_buf and best_buf instead?

Thanks, that's a good idea.  Updated.


- Paul

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> hmm, that memcpy is costly. Just swap tmp_buf and best_buf instead?

Thanks, that's a good idea.  Updated.


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> .. and you should delay the second allocation to when it is needed. After swapping ptrs, test if
> tmp_buf is NULL and kmalloc if so.

Hmm.  You are thinking of a GFP_ATOMIC allocation?  Seems best to avoid 
those?


- Paul

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: CONFIG_PREEMPT and JFFS2 oops

[Paul Walmsley]

On Thu, 26 Jan 2012, Joakim Tjernlund wrote:

> .. and you should delay the second allocation to when it is needed. After swapping ptrs, test if
> tmp_buf is NULL and kmalloc if so.

Hmm.  You are thinking of a GFP_ATOMIC allocation?  Seems best to avoid 
those?


- Paul

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

Paul Walmsley <paul@pwsan.com> wrote on 2012/01/28 10:51:26:
>
> On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
>
> > .. and you should delay the second allocation to when it is needed. After swapping ptrs, test if
> > tmp_buf is NULL and kmalloc if so.
>
> Hmm.  You are thinking of a GFP_ATOMIC allocation?  Seems best to avoid
> those?

No, just stick the kmalloc after the unlock:

			spin_unlock(&jffs2_compressor_list_lock);
			if (!tmp_buf) {
				tmp_buf = kmalloc(...);
				<error handling if still NULL>
			}
			*datalen  = orig_slen;
			*cdatalen = orig_dlen;
			compr_ret = this->compress(data_in, tmp_buf, datalen, cdatalen, NULL);


That way you also skip a second kmalloc if the list only holds one compressor and
you don't need GFP_ATOMIC


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: CONFIG_PREEMPT and JFFS2 oops

[Joakim Tjernlund]

Paul Walmsley <paul@pwsan.com> wrote on 2012/01/28 10:51:26:
>
> On Thu, 26 Jan 2012, Joakim Tjernlund wrote:
>
> > .. and you should delay the second allocation to when it is needed. After swapping ptrs, test if
> > tmp_buf is NULL and kmalloc if so.
>
> Hmm.  You are thinking of a GFP_ATOMIC allocation?  Seems best to avoid
> those?

No, just stick the kmalloc after the unlock:

			spin_unlock(&jffs2_compressor_list_lock);
			if (!tmp_buf) {
				tmp_buf = kmalloc(...);
				<error handling if still NULL>
			}
			*datalen  = orig_slen;
			*cdatalen = orig_dlen;
			compr_ret = this->compress(data_in, tmp_buf, datalen, cdatalen, NULL);


That way you also skip a second kmalloc if the list only holds one compressor and
you don't need GFP_ATOMIC