sysfs reclaim crash

PATCH In review Maneesh Soni 3 participants 6 messages Mar 24, 2007 – Apr 7, 2007 6 messages not in the archive Driver core

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Maneesh Soni

author

2007-03-24 03:05 UTC #6 lore raw

Quoted 77 lines · Ethan Solomita

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

Quoted 6 lines

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,

Thank you very much for the crash data. This is helpful. Could you please test the appended patch. Here we avoid modifying s_dentry in sysfs_d_iput() and also uses iunique() in sysfs_readdir() instead of accessing s_dentry to get the inode number.

Though I was not able to recreate this race without the patch, but I am running this patch successfully for more than 6 hrs now with the following loops parallely on a 4-way SMP system. So, at least it has not done anything bad.

1. while true; do insmod drivers/net/dummy.ko ; rmmod dummy; done
2. [root@llm01 net]# pwd
/sys/class/net
[root@llm01 net]# while true; do find | xargs cat > /dev/null; done
3. [root@llm01 sys]# pwd
/sys
[root@llm01 sys]# while true; do ls -liR; done

Also, CC-ed Viro and lkml for feedback.

Thanks
Maneesh

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

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

o This also avoids the now unnecessary s_dentry in sysfs_d_iput().

Signed-off-by Maneesh Soni <[email protected]>

---

 linux-2.6.21-rc4-git8-maneesh/fs/sysfs/dir.c |    6 +-----
 1 files changed, 1 insertion(+), 5 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~fix-sysfs-reclaim-race +1 -5

diff -puN fs/sysfs/dir.c~fix-sysfs-reclaim-race fs/sysfs/dir.c
@@ -20,7 +20,6 @@ static void sysfs_d_iput(struct dentry *
 
 	if (sd) {
 		BUG_ON(sd->s_dentry != dentry);
-		sd->s_dentry = NULL;
 		sysfs_put(sd);
 	}
 	iput(inode);

@@ -538,10 +537,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)

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

Ethan Solomita

2007-03-27 18:28 UTC #7 lore raw

Hi Maneesh -- I will start testing with the patch you provided. If you
come up with any further issues please let me know. Also, if you could suggest some additional BUG() lines that I could insert I would appreciate it. Since the bug is hard to reproduce, it may be easier to catch a race condition in the making via BUG() than an actual failure due to a race condition.

Thanks!
-- Ethan

Maneesh Soni

author

2007-03-27 18:49 UTC #8 lore raw

Quoted 7 lines · Ethan Solomita

Hi Maneesh -- I will start testing with the patch you provided. If you come up with any further issues please let me know. Also, if you could
suggest some additional BUG() lines that I could insert I would appreciate it. Since the bug is hard to reproduce, it may be easier to catch a race condition in the making via BUG() than an actual failure due to a race condition.

Hi Ethan,

Thanks for testing. The BUG_ON in sysfs_d_iput() is still there to catch the first race we saw. And the second one should not occur as now we are not using the s_dentry in sysfs_readdir().

Regards,
Maneesh

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

Maneesh Soni

author

2007-04-03 07:38 UTC #9 lore raw

PATCH 1/1 fix sysfs_readdir oops (was Re: sysfs reclaim crash)

In reply to Maneesh Soni

Quoted 78 lines · Maneesh Soni

Quoted 77 lines · Ethan Solomita
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

Quoted 8 lines

Quoted 6 lines
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 <[email protected]>

---

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

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~fix-sysfs_readdir-oops +1 -4

diff -puN fs/sysfs/dir.c~fix-sysfs_readdir-oops fs/sysfs/dir.c
@@ -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)

Ethan Solomita

2007-04-03 20:47 UTC #10 lore raw

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

Quoted 4 lines · Maneesh Soni

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.

If you could find some additional places in sysfs code to add new
BUG() checks I'd appreciate it. Especially if it turns out that you can't reproduce it, I'd like to have as many asserts as is reasonable.
-- Ethan

Tejun Heo

2007-04-07 08:31 UTC #11 lore raw

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

In reply to Maneesh Soni

Hello, Maneesh.

Quoted 7 lines · Maneesh Soni

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().

Quoted 8 lines

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.

This isn't correct as i_unique() assumes the inode is in inode hash table which isn't true for sysfs. This can result in duplicate inode numbers. Please take a look at the following alternative fix.

http://article.gmane.org/gmane.linux.kernel/513325

Thanks.

Signature · 1 line

tejun