JFFS2 oops when writing to two partitions simultaneously

[Joakim Tjernlund]

Orjan Friberg <of at flatfrog.com> wrote on 2012/01/26 13:51:30:

> From: Orjan Friberg <of at flatfrog.com>
> To: Joakim Tjernlund <joakim.tjernlund at transmode.se>
> Cc: "linux-mtd at lists.infradead.org" <linux-mtd at lists.infradead.org>
> Date: 2012/01/26 13:51
> Subject: Re: JFFS2 oops when writing to two partitions simultaneously
>
> On 01/26/2012 12:53 PM, Joakim Tjernlund wrote:
> >          /* 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 2 threads are competing here, I don't think you can drop the spin lock
> > temporarily as this routine do.
>
> Agreed.  Both the freeing of this->compr_buf and the usage of it when
> calling the compressor looks weird (because another process holding the
> lock could decide that the buffer is too small and allocate a new one):

Yes, possibly there is some mutex protecting this?

>
>     spin_unlock(&jffs2_compressor_list_lock);
>     *datalen  = orig_slen;
>     *cdatalen = orig_dlen;
>     compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
>     spin_lock(&jffs2_compressor_list_lock);
>
>
> I'm not sure I'm crazy about the allocation either, come to think of it:
>
> 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;
>    }
> }
>
> Even though we hold the lock when assigning the new buffer, things could
> have been changed while we're doing the kmalloc.  In this case, maybe
> just dropping the unlock/lock and allocating with GFP_ATOMIC would solve it.

The freeing is broken too:
				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;

First kfree then assign NULL,0? Thats broken either way

Anyhow, I think it is stupid (and probably buggy) to have kfree and kmalloc
as separate. Why is it not done at the same time?

>
> I'm not sure I see why compr_buf has to belong to the compressor.  To
> not have to kmalloc a buffer each and every time?

Probably