jffs2 mutex problem

jffs2 mutex problem

[Keijiro Yano]

Hello,

I find out the problem related to the mutex lock in JFFS2.
I always use Linux 2.6.10, but Linux 2.6.14 also has a same
problem, I think.

jffs2_create() calls jffs2_new_inode() that allocates new
"jffs2_inode_info" with jffs2_alloc_inode().
Generally, in this case, jffs2_i_init_once(), which is the
constructor for jffs2_inode_cachep, will be called and
initialize the mutex with lock state by the below code.

static void jffs2_i_init_once(void * foo, kmem_cache_t * cachep, unsigned 
long flags)
{
struct jffs2_inode_info *ei = (struct jffs2_inode_info *) foo;

if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
    SLAB_CTOR_CONSTRUCTOR) {
init_MUTEX_LOCKED(&ei->sem);

But, the constructor is called by kmem_cache_alloc()
only when no active objects are left in a cache.

Therefore, sometimes jffs2_i_init_once() will not be
called after jffs2_alloc_inode() is done.

So, jffs2_do_create(), which is called by jffs2_create(), will
do "up(&f->sem)" in unlocking state.

I try to fix this problem with the following patch. Please
review it and/or give me your comments.

--- fs/jffs2/os-linux.h 10 Feb 2005 10:15:41 -0000 1.1.1.1
+++ fs/jffs2/os-linux.h 9 Nov 2005 08:36:13 -0000
@@ -93,8 +93,8 @@
  f->usercompr = 0;
 #else
  memset(f, 0, sizeof(*f));
- init_MUTEX_LOCKED(&f->sem);
 #endif
+ init_MUTEX_LOCKED(&f->sem);
 }

 #define jffs2_is_readonly(c) (OFNI_BS_2SFFJ(c)->s_flags & MS_RDONLY)

--- fs/jffs2/super.c 10 Feb 2005 10:15:41 -0000 1.1.1.1
+++ fs/jffs2/super.c 9 Nov 2005 08:36:13 -0000
@@ -51,7 +51,7 @@

  if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
      SLAB_CTOR_CONSTRUCTOR) {
- init_MUTEX_LOCKED(&ei->sem);
  inode_init_once(&ei->vfs_inode);
  }
 }

Thanks,
Keijiro Yano

--------------------------------------
Know more about Breast Cancer
http://pr.mail.yahoo.co.jp/pinkribbon/

Re: jffs2 mutex problem

[Artem B. Bityutskiy]

Keijiro Yano wrote:
> Hello,
> 
> I find out the problem related to the mutex lock in JFFS2.
> I always use Linux 2.6.10, but Linux 2.6.14 also has a same
> problem, I think.
> 
> jffs2_create() calls jffs2_new_inode() that allocates new
> "jffs2_inode_info" with jffs2_alloc_inode().
> Generally, in this case, jffs2_i_init_once(), which is the
> constructor for jffs2_inode_cachep, will be called and
> initialize the mutex with lock state by the below code.
> 
> static void jffs2_i_init_once(void * foo, kmem_cache_t * cachep, 
> unsigned long flags)
> {
> struct jffs2_inode_info *ei = (struct jffs2_inode_info *) foo;
> 
> if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
>    SLAB_CTOR_CONSTRUCTOR) {
> init_MUTEX_LOCKED(&ei->sem);
> 
> But, the constructor is called by kmem_cache_alloc()
> only when no active objects are left in a cache.
> 
> Therefore, sometimes jffs2_i_init_once() will not be
> called after jffs2_alloc_inode() is done.
> 
> So, jffs2_do_create(), which is called by jffs2_create(), will
> do "up(&f->sem)" in unlocking state.
Hmm, really. IMO it is a bug to initialize the mutex as locked in the 
constructor while the mutex is left unlocked when it is being freed.

> I try to fix this problem with the following patch. Please
> review it and/or give me your comments.
> 
> --- fs/jffs2/os-linux.h 10 Feb 2005 10:15:41 -0000 1.1.1.1
> +++ fs/jffs2/os-linux.h 9 Nov 2005 08:36:13 -0000
> @@ -93,8 +93,8 @@
>  f->usercompr = 0;
> #else
>  memset(f, 0, sizeof(*f));
> - init_MUTEX_LOCKED(&f->sem);
> #endif
> + init_MUTEX_LOCKED(&f->sem);
> }
> 
> #define jffs2_is_readonly(c) (OFNI_BS_2SFFJ(c)->s_flags & MS_RDONLY)
> 
> --- fs/jffs2/super.c 10 Feb 2005 10:15:41 -0000 1.1.1.1
> +++ fs/jffs2/super.c 9 Nov 2005 08:36:13 -0000
> @@ -51,7 +51,7 @@
> 
>  if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
>      SLAB_CTOR_CONSTRUCTOR) {
> - init_MUTEX_LOCKED(&ei->sem);
>  inode_init_once(&ei->vfs_inode);
>  }
> }

Well, the patch is not against the latest MTD CVS snapshot which is bad.

The right solution, IMO, would be:

1. use init_MUTEX() in the slab constructor for jffs2_inode_info() objects.
2. add explicit down(&f->sem) everywhere after jffs2_new_inode().

This is right because the "constructed" state of the inode is when the 
mutex is initialized and unlocked. FYI, here is a good article where you 
can read about the idea of constructors in the slab cache: 
http://srl.cs.jhu.edu/courses/600.418/SlabAllocator.pdf


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

Re: jffs2 mutex problem

[Keijiro Yano]

Hi,

> Well, the patch is not against the latest MTD CVS snapshot which is bad.

Thanks for your information.
I modify my patch to fix this problem.


--- fs.c 10 Feb 2005 10:15:41 -0000 1.1.1.1
+++ fs.c 14 Nov 2005 01:59:24 -0000
@@ -236,6 +236,8 @@
  c = JFFS2_SB_INFO(inode->i_sb);
 
  jffs2_init_inode_info(f);
+
+ down(&f->sem);
  
  ret = jffs2_do_read_inode(c, f, inode->i_ino, &latest_node);
 
@@ -403,6 +405,8 @@
  f = JFFS2_INODE_INFO(inode);
  jffs2_init_inode_info(f);
 
+ down(&f->sem);
+ 
  memset(ri, 0, sizeof(*ri));
  /* Set OS-specific defaults for new inodes */
  ri->uid = cpu_to_je16(current->fsuid);

--- super.c 10 Feb 2005 10:15:41 -0000 1.1.1.1
+++ super.c 14 Nov 2005 01:36:34 -0000
@@ -51,7 +51,7 @@
 
  if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
      SLAB_CTOR_CONSTRUCTOR) {
- init_MUTEX_LOCKED(&ei->sem);
+ init_MUTEX(&ei->sem);
  inode_init_once(&ei->vfs_inode);
  }
 }

--------------------------------------
Know more about Breast Cancer
http://pr.mail.yahoo.co.jp/pinkribbon/

Re: jffs2 mutex problem

[Artem B. Bityutskiy]

On Mon, 2005-11-14 at 21:55 +0900, Keijiro Yano wrote:
> Hi,
> 
> > Well, the patch is not against the latest MTD CVS snapshot which is bad.
> 
> Thanks for your information.
> I modify my patch to fix this problem.
> 
Is this against the latest CVS?
Can you confirm that you tested the patch?
What about to fix ecos as well?

$ grep -r jffs2_init_inode_info *
ecos/src/fs-ecos.c:     jffs2_init_inode_info(f);
ecos/src/fs-ecos.c:     jffs2_init_inode_info(f);

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

Re: jffs2 mutex problem

[Keijiro Yano]

> Is this against the latest CVS?

It is for Linux 2.6.10.
But, based on my source level investigation, it can 
be applied for the latest CVS.

> Can you confirm that you tested the patch?

I have tested it only on Linux 2.6.10.

> What about to fix ecos as well?

I think that ecos doesn't have this problem, because 
ecos initializes the locked mutex in jffs2_init_inode_info().

Best Regards,
Keijiro Yano


--------------------------------------
Know more about Breast Cancer
http://pr.mail.yahoo.co.jp/pinkribbon/

Re: jffs2 mutex problem

[Josh Boyer]

On 11/15/05, Keijiro Yano <keijiro_yano@yahoo.co.jp> wrote:
> > Is this against the latest CVS?
>
> It is for Linux 2.6.10.
> But, based on my source level investigation, it can
> be applied for the latest CVS.

Somehow I doubt that.  There was recently a large whitespace cleanup
in the latest CVS.  I'm sure a patch that doesn't take that into
account will either throw lots of rejects or introduce whitespace
stuff that was just cleaned up.

Remember, you should diff against the latest.  You can't see the
whitespace stuff unless you have some option in your editor turned on.

josh

Re: jffs2 mutex problem

[Artem B. Bityutskiy]

Keijiro Yano wrote:
> It is for Linux 2.6.10.
> But, based on my source level investigation, it can be applied for the 
> latest CVS.
Ok.

> I have tested it only on Linux 2.6.10.
Ok.

> I think that ecos doesn't have this problem, because ecos initializes 
> the locked mutex in jffs2_init_inode_info().
Hmm, I do you mean that Ecos's jffs2_init_inode_info() is not a slab
cache constructor so the bug does not exist there? Probably you're right.

Thanks. I think I'll commit this patch if people are OK.

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

Re: jffs2 mutex problem

[Artem B. Bityutskiy]

Keijiro Yano wrote:
> It is for Linux 2.6.10.
> But, based on my source level investigation, it can be applied for the 
> latest CVS.

I've committed your patch to CVS, thanks.

Note: the patch wasn't applicable to the CVS code and I tweaked it manually.

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

Re: jffs2 mutex problem

[Keijiro Yano]

> I've committed your patch to CVS, thanks.
>
> Note: the patch wasn't applicable to the CVS code and I tweaked it 
> manually.

Thank you !
And sorry for inconvenience.

Best Regards,
Keijiro Yano

--------------------------------------
STOP HIV/AIDS.
Yahoo! JAPAN Redribbon Campaign 2005
http://pr.mail.yahoo.co.jp/redribbon/