[PATCH] fix sysfs_readdir oops (was Re: sysfs reclaim crash)

[Maneesh Soni]

On Sat, Mar 24, 2007 at 08:35:35AM +0530, Maneesh Soni wrote:
> On Fri, Mar 23, 2007 at 12:43:27PM -0700, Ethan Solomita wrote:
> >     I ran stress testing overnight and came up with a similar failure
> > (s_dentry == NULL) but in a different location. A NULL pointer
> > dereference happened in sysfs_readdir():
> > 
> > |                                if (next->s_dentry)
> > |                                        ino =
> > next->s_dentry->d_inode->i_ino;
> > 
> >     It seems that d_inode was NULL. I don't have the pointer to d_inode
> > to look at, but I have next and, lo and behold, its s_dentry is now
> > NULL, which it clearly wasn't when the if-clause above ran.
> > 
> >     I tried to reconstruct the sysfs_dirents starting with "next". I
> > filled in all the structure contents that I had data for:
> > 
> > sysfs_dirent 0xffff81000fc61690:
> > s_count         1
> > s_sibling       ffff81000fc616e8 / ffff81000e0c7468
> > s_children      ffff81000fc616a8 / ffff81000fc616a8
> > s_element       ffff81000f4ad1b0
> >   DOR__ATA1RTS
> >   ffffffff8800b600
> >   124
> > s_type          4
> > s_mode          8124
> > s_dentry        NULL
> > s_iattr         NULL
> > s_event         0
> > 
> > s_sibling.prev:
> > s_count         1
> > s_sibling       ffff81000fc61738 / ffff81000fc61698
> > s_children      ffff81000fc616f8 / ffff81000fc616f8
> > s_element       ffff81000f4ad148
> > s_type          4
> > s_mode          8124
> > <unknown>
> > 
> > s_sibling.next:
> > s_count         1
> > s_sibling       ffff81000fc61698 / ffff81000fc61648
> > s_children      ffff81000e0c7478 / ffff81000e0c7478
> > s_element       NULL
> > s_type          0
> > s_mode          0
> > s_dentry        NULL
> > s_iattr         NULL
> > s_event         0
> > 
> > s_sibling.next.next:
> > s_count         1
> > s_sibling       ffff81000e0c7468 / ffff81000fc615f8
> > s_children      ffff81000fc61658 / ffff81000fc61658
> > s_element       ffff81000f4ad218
> >   DOR__ATA1RTS
> >   ffffffff8800b600
> >   124
> > s_type          4
> > s_mode          8124
> > s_dentry        NULL
> > s_iattr         NULL
> > s_event         0
> > 
> > s_sibling.next.next.next:
> > s_count         1
> > s_sibling       ffff81000fc61648 / ffff81000fc610f8
> > s_children      ffff81000fc61608 / ffff81000fc61608
> > s_element       ffff81000f4ad280
> >   CK??ATCR
> >   ffffffff8800b600
> >   124
> > s_type          4
> > s_mode          8124
> > s_dentry        NULL
> > s_iattr         NULL
> > s_event         0
> > 
> >     I should acknowledge that this is based upon 2.6.18 with some newer
> > code backported. If there are fixes since 2.6.18 that we should know
> > about I can try backporting them into our kernel.
> > 
> >     Thanks,
> >     -- Ethan
> 
> 

Hi Ethan / Andrew,

I have modified the previous patch (which was dropped from -mm) and now keeping
the statement making s_dentry as NULL in sysfs_d_iput(), so this should
_safely_ fix sysfs_readdir() oops. 

Please see the other mail for sysfs_d_iput() BUG hit.

Thanks
Maneesh

--
Maneesh Soni
Linux Technology Center,
IBM India Systems and Technology Lab, 
Bangalore, India




o sysfs_d_iput() is invoked in dentry reclaim path under memory pressure. This
  happens without i_mutex. It also nullifies s_dentry to indicate that
  the associated dentry is evicted. sysfs_readdir() accesses the s_dentry,
  and gets the inode number from the associated dentry->d_inode, if 
  there is one, else it invokes iunique(). This can create a race situation,
  and crash while accessing the d_inode in sysfs_readdir(). 

o The race happens when the dentry is getting reclaimed and detached from
  the corresponding sysfs_dirent though sysfs_dirent is still a valid
  node. Accessing dentry fields are ok as it is under RCU but the inode is
  not hence we may see oops accessing dentry->d_inode->i_no.

o The following patch always use i_unique() to get the inode number in
  sysfs_readdir. This is ok as sysfs doesnot have permanent inode numbering.
  It could be slower but avoids the oops. 



Signed-off-by: Maneesh Soni <maneesh@xxxxxxxxxx>
---

 linux-2.6.21-rc5-mm3-maneesh/fs/sysfs/dir.c |    5 +----
 1 files changed, 1 insertion(+), 4 deletions(-)

diff -puN fs/sysfs/dir.c~fix-sysfs_readdir-oops fs/sysfs/dir.c
--- linux-2.6.21-rc5-mm3/fs/sysfs/dir.c~fix-sysfs_readdir-oops	2007-04-03 10:39:52.000000000 +0530
+++ linux-2.6.21-rc5-mm3-maneesh/fs/sysfs/dir.c	2007-04-03 10:39:52.000000000 +0530
@@ -538,10 +538,7 @@ static int sysfs_readdir(struct file * f
 
 				name = sysfs_get_name(next);
 				len = strlen(name);
-				if (next->s_dentry)
-					ino = next->s_dentry->d_inode->i_ino;
-				else
-					ino = iunique(sysfs_sb, 2);
+				ino = iunique(sysfs_sb, 2);
 
 				if (filldir(dirent, name, len, filp->f_pos, ino,
 						 dt_type(next)) < 0)
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/