Re: JFFS2 oops when writing to two partitions simultaneously

Joakim Tjernlund
2012-01-26T13:17:06Z

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

> From: Orjan Friberg <of@flatfrog.com>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Cc: "linux-mtd@lists.infradead.org" <linux-mtd@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