diff mbox

CONFIG_PREEMPT and JFFS2 oops

Message ID alpine.DEB.2.00.1201260953550.32667@utopia.booyaka.com
State New, archived
Headers show

Commit Message

Paul Walmsley Jan. 26, 2012, 4:57 p.m. UTC 
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(-)

Comments

Joakim Tjernlund Jan. 26, 2012, 5:33 p.m. UTC | #1 
> 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
Orjan Friberg Jan. 26, 2012, 5:54 p.m. UTC | #2 
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).
Joakim Tjernlund Jan. 26, 2012, 8:01 p.m. UTC | #3 
> >
> > 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
Paul Walmsley Jan. 28, 2012, 9:50 a.m. UTC | #4 
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
Paul Walmsley Jan. 28, 2012, 9:51 a.m. UTC | #5 
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
Joakim Tjernlund Jan. 28, 2012, 2:42 p.m. UTC | #6 
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
diff mbox

Patch

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;