inode->i_wb_list corruption.

DISCUSSION Dave Jones 7 participants 16 messages Mar 6, 2012 – Mar 13, 2012 VFS

Share & tools

Dave Jones

author

2012-03-06 18:51 UTC #0 lore raw

We've had three separate reports against 3.2.x recently where the linked list debugging is getting tripped up by the prev->next pointer being null instead of pointing to the current list entry while walking the i_wb_list

Call traces are slightly different each time, but all end up walking i_wb_list in dput -> d_kill -> i_put -> evict -> inode_wb_list_del

What protects that list ? It looks to be just bdi->wb.list_lock ?

full reports at:
https://bugzilla.redhat.com/show_bug.cgi?id=784741
https://bugzilla.redhat.com/show_bug.cgi?id=799229
https://bugzilla.redhat.com/show_bug.cgi?id=799692

Dave

Jan Kara

reviewer

2012-03-06 21:03 UTC #1 lore raw

Quoted 7 lines · Dave Jones

We've had three separate reports against 3.2.x recently where the linked list debugging is getting tripped up by the prev->next pointer being null instead of pointing to the current list entry while walking the i_wb_list
Call traces are slightly different each time, but all end up walking i_wb_list in dput -> d_kill -> i_put -> evict -> inode_wb_list_del

Quoted 7 lines

What protects that list ? It looks to be just bdi->wb.list_lock ?
full reports at:
https://bugzilla.redhat.com/show_bug.cgi?id=784741
https://bugzilla.redhat.com/show_bug.cgi?id=799229
https://bugzilla.redhat.com/show_bug.cgi?id=799692

Hum, interesting! I'd guess this might be caused by f758eeab - adding
Fengguang and Christoph to CC. But I'm really failing to see how this could happen but interesting thing is that in two of the three cases the files are on virtual filesystems (once cgroup, once sysfs). These both use noop_backing_dev_info.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Fengguang Wu

2012-03-07 07:31 UTC #2 lore raw

Quoted 15 lines · Jan Kara

Quoted 7 lines · Dave Jones
We've had three separate reports against 3.2.x recently where the linked list debugging is getting tripped up by the prev->next pointer being null instead of pointing to the current list entry while walking the i_wb_list
Call traces are slightly different each time, but all end up walking i_wb_list in dput -> d_kill -> i_put -> evict -> inode_wb_list_del
Quoted 7 lines
What protects that list ? It looks to be just bdi->wb.list_lock ?
full reports at:
https://bugzilla.redhat.com/show_bug.cgi?id=784741
https://bugzilla.redhat.com/show_bug.cgi?id=799229
https://bugzilla.redhat.com/show_bug.cgi?id=799692

Quoted 5 lines

Hum, interesting! I'd guess this might be caused by f758eeab - adding
Fengguang and Christoph to CC. But I'm really failing to see how this could happen but interesting thing is that in two of the three cases the files are on virtual filesystems (once cgroup, once sysfs). These both use noop_backing_dev_info.

sysfs/cgroup forgot to init inode->i_wb_list?

This simplified fix inits it in inode_init_always().

The better fix would be to add init_once to sysfs or perhaps fix sysfs_get_inode()/cgroup_new_inode().

Thanks,
Fengguang

---
 fs/inode.c |    1 +
 1 file changed, 1 insertion(+)

fs/inode.c +1 -0

@@ -193,6 +193,7 @@ int inode_init_always(struct super_block
 	inode->i_private = NULL;
 	inode->i_mapping = mapping;
 	INIT_LIST_HEAD(&inode->i_dentry);	/* buggered by rcu freeing */
+	INIT_LIST_HEAD(&inode->i_wb_list);
 #ifdef CONFIG_FS_POSIX_ACL
 	inode->i_acl = inode->i_default_acl = ACL_NOT_CACHED;
 #endif

Jan Kara

reviewer

2012-03-07 10:42 UTC #3 lore raw

Quoted 16 lines · Wu Fengguang

Quoted 15 lines · Jan Kara
Quoted 14 lines · Dave Jones
We've had three separate reports against 3.2.x recently where the linked list debugging is getting tripped up by the prev->next pointer being null instead of pointing to the current list entry while walking the i_wb_list
Call traces are slightly different each time, but all end up walking i_wb_list in dput -> d_kill -> i_put -> evict -> inode_wb_list_del
What protects that list ? It looks to be just bdi->wb.list_lock ?
full reports at:
https://bugzilla.redhat.com/show_bug.cgi?id=784741
https://bugzilla.redhat.com/show_bug.cgi?id=799229
https://bugzilla.redhat.com/show_bug.cgi?id=799692

Quoted 7 lines

Quoted 5 lines
Hum, interesting! I'd guess this might be caused by f758eeab - adding
Fengguang and Christoph to CC. But I'm really failing to see how this could happen but interesting thing is that in two of the three cases the files are on virtual filesystems (once cgroup, once sysfs). These both use noop_backing_dev_info.
sysfs/cgroup forgot to init inode->i_wb_list?

Umm, it's not *that* simple I'd say. E.g. sysfs doesn't provide
alloc_inode() method so we use inode_cachep for allocations. And that cache is configured to use inode_init_once().

Also note that the error message is:
list_del corruption. prev->next should be ffff8801c2f41b18, but was (null)

Which means that our inode had correct i_wb_list.prev but the previous inode had NULL in i_wb_list.next. But that means that both inodes were linked into the list at some point. So it does not seem like an initialization issue to me...

Honza

Quoted 10 lines

fs/inode.c +1 -0

@@ -193,6 +193,7 @@ int inode_init_always(struct super_block
 	inode->i_private = NULL;
 	inode->i_mapping = mapping;
 	INIT_LIST_HEAD(&inode->i_dentry);	/* buggered by rcu freeing */
+	INIT_LIST_HEAD(&inode->i_wb_list);
 #ifdef CONFIG_FS_POSIX_ACL
 	inode->i_acl = inode->i_default_acl = ACL_NOT_CACHED;
 #endif

Yang Bai

2012-03-09 08:35 UTC #4 lore raw

Quoted 8 lines · Jan Kara

On Tue 06-03-12 23:26:08, Wu Fengguang wrote:
Umm, it's not *that* simple I'd say. E.g. sysfs doesn't provide alloc_inode() method so we use inode_cachep for allocations. And that cache is configured to use inode_init_once().
Also note that the error message is:
list_del corruption. prev->next should be ffff8801c2f41b18, but was (null)

Quoted 7 lines

Which means that our inode had correct i_wb_list.prev but the previous inode had NULL in i_wb_list.next. But that means that both inodes were linked into the list at some point. So it does not seem like an initialization issue to me...
Honza

I still want to know how to reproduce this bug. I add the following patch to the kernel fc-16 3.2.9-1

fs/fs-writeback.c +0 -0

index 5b4a936..568ed0a 100644
@@ -192,8 +192,13 @@ void bdi_start_background_writeback(struct

backing_dev_info *bdi)
void inode_wb_list_del(struct inode *inode)
{
struct backing_dev_info *bdi = inode_to_bdi(inode);
+ struct list_head *pos;

spin_lock(&bdi->wb.list_lock);
+ list_for_each(pos, &inode->i_wb_list) {
+ printk(KERN_EMERG "list entry: %p; next: %p; prev: %p",
+ pos, pos->next, pos->prev);
+ }
list_del_init(&inode->i_wb_list);
spin_unlock(&bdi->wb.list_lock);
}

So on every inode_wb_list_del, it will show the whole list.

and Doing while true; do touch a && rm -f a; done for almost one day without any problem.

So How to reproduce it??

Thanks,
Yang

Dave Jones

author

2012-03-09 14:57 UTC #5 lore raw

Quoted 9 lines · Yang Bai

I still want to know how to reproduce this bug. I add the following patch to the kernel fc-16 3.2.9-1
So on every inode_wb_list_del, it will show the whole list.
and Doing while true; do touch a && rm -f a; done for almost one day without any problem.
So How to reproduce it??

If it was that easy, I'd be bisecting it by now ;-)

This, like a bunch of other really weird bugs that we have no explanation for, only seems to be being hit by a small minority of users.

One common thing seems to be that they were all quad core intel boxes, with i915 graphics.

We have some reports of i915 causing memory corruption after suspend/hibernate, but none of these reports mention whether they've done that (I just asked).

Dave

Dave Jones

author

2012-03-09 15:20 UTC #6 lore raw

Quoted 14 lines · Dave Jones

Quoted 9 lines · Yang Bai
I still want to know how to reproduce this bug. I add the following patch to the kernel fc-16 3.2.9-1
So on every inode_wb_list_del, it will show the whole list.
and Doing while true; do touch a && rm -f a; done for almost one day without any problem.
So How to reproduce it??
If it was that easy, I'd be bisecting it by now ;-)

Quoted 8 lines

This, like a bunch of other really weird bugs that we have no explanation for, only seems to be being hit by a small minority of users.
One common thing seems to be that they were all quad core intel boxes, with i915 graphics.
We have some reports of i915 causing memory corruption after suspend/hibernate, but none of these reports mention whether they've done that (I just asked).

And with that, this arrived..
https://bugzilla.redhat.com/show_bug.cgi?id=788433#c3

I'm leaning strongly towards believing this is yet another case of i915 corrupting memory on resume.

Dave

Yang Bai

2012-03-09 16:14 UTC #7 lore raw

Quoted 6 lines · Dave Jones

And with that, this arrived..
https://bugzilla.redhat.com/show_bug.cgi?id=788433#c3
I'm leaning strongly towards believing this is yet another case of i915 corrupting memory on resume.

Nice catch. I am wondering
1) why all lists being affected and
2) why all list_head's prev being set to NULL.

Any ideas?

Thanks,
Yang

Dave Jones

author

2012-03-09 18:00 UTC #8 lore raw

(trimmed cc)

Quoted 4 lines · Yang Bai

Quoted 3 lines · Dave Jones
And with that, this arrived..
https://bugzilla.redhat.com/show_bug.cgi?id=788433#c3

Quoted 8 lines

Quoted 2 lines
I'm leaning strongly towards believing this is yet another case of i915 corrupting memory on resume.
Nice catch. I am wondering
1) why all lists being affected and
2) why all list_head's prev being set to NULL.
Any ideas?

This is probably the same bug: https://bugzilla.kernel.org/show_bug.cgi?id=37142
Petr noticed that the corruption is 32 bytes getting zeroed at the beginning of a page.

I think this may be responsible for a lot of different bugs that we've had reported.

i915_drm_thaw is a deep nest of functions though, so this is going to be hard to track down where that write is coming from. Because the corruption seems to happen to pages that are already allocated, we probably can't even rely on DEBUG_PAGEALLOC, though it might be worth trying.

Dave

Keith Packard

2012-03-09 20:08 UTC #9 lore raw

<#part sign=pgpmime>

Quoted 4 lines · Dave Jones

i915_drm_thaw is a deep nest of functions though, so this is going to be hard to track down where that write is coming from. Because the corruption seems to happen to pages that are already allocated, we probably can't even rely on DEBUG_PAGEALLOC, though it might be worth trying.

I'm worried that the write is coming through the GTT, which would make sense as these look like pixel values. If this is on Ironlake (core I3-I7 first gen), we know there are issues when VT-d is enabled, and the work-around for that doesn't appear to be in place for the hibernate resume case.

Signature · 1 line

[email protected]

Josh Boyer

2012-03-09 20:19 UTC #10 lore raw

Quoted 8 lines · Keith Packard

<#part sign=pgpmime>
Quoted 4 lines · Dave Jones
i915_drm_thaw is a deep nest of functions though, so this is going to be hard to track down where that write is coming from. Because the corruption seems to happen to pages that are already allocated, we probably can't even rely on DEBUG_PAGEALLOC, though it might be worth trying.

Quoted 5 lines

I'm worried that the write is coming through the GTT, which would make sense as these look like pixel values. If this is on Ironlake (core I3-I7 first gen), we know there are issues when VT-d is enabled, and the work-around for that doesn't appear to be in place for the hibernate resume case.

Is the VT-d issue something in the hardware itself, or do you mean if you have it enabled in the kernel? We've had the intel IOMMU disabled by default in the Fedora kernels for a while now. At least since before 3.2 was released.

josh

Keith Packard

2012-03-09 22:44 UTC #11 lore raw

<#part sign=pgpmime>

Quoted 4 lines · Josh Boyer

Is the VT-d issue something in the hardware itself, or do you mean if you have it enabled in the kernel? We've had the intel IOMMU disabled by default in the Fedora kernels for a while now. At least since before 3.2 was released.

I don't know for sure; David Woodhouse gave a scary presentation yesterday that makes me unsure of what happens when IOMMU is disabled in the kernel, given that much of the hardware is setup by the BIOS.

Signature · 1 line

[email protected]

Josh Boyer

2012-03-12 21:13 UTC #12 lore raw

Quoted 11 lines · Keith Packard

<#part sign=pgpmime>
Quoted 4 lines · Josh Boyer
Is the VT-d issue something in the hardware itself, or do you mean if you have it enabled in the kernel? We've had the intel IOMMU disabled by default in the Fedora kernels for a while now. At least since before 3.2 was released.
I don't know for sure; David Woodhouse gave a scary presentation yesterday that makes me unsure of what happens when IOMMU is disabled in the kernel, given that much of the hardware is setup by the BIOS.

Is that presentation something that could be shared? I have to say, hearing that doesn't really inspire confidence in either the IOMMU or the kernel.

josh

David Woodhouse

2012-03-12 21:27 UTC #13 lore raw

Quoted 9 lines · Josh Boyer

Quoted 8 lines · Keith Packard
<#part sign=pgpmime>
Quoted 4 lines · Josh Boyer
Is the VT-d issue something in the hardware itself, or do you mean if you have it enabled in the kernel? We've had the intel IOMMU disabled by default in the Fedora kernels for a while now. At least since before 3.2 was released.

Quoted 7 lines

Quoted 3 lines
I don't know for sure; David Woodhouse gave a scary presentation yesterday that makes me unsure of what happens when IOMMU is disabled in the kernel, given that much of the hardware is setup by the BIOS.
Is that presentation something that could be shared? I have to say, hearing that doesn't really inspire confidence in either the IOMMU or the kernel.

It was mostly just a rant about the design mistakes we made with the IOMMU — in particular giving the BIOS as much rope as possible for it to hang us with.

If the BIOS exposes the IOMMU but the OS chooses not to enable it, I don't believe there's any problem with that.

Signature · 1 line

dwmw2

Dave Jones

author

2012-03-12 23:26 UTC #14 lore raw

In reply to Keith Packard

Quoted 8 lines · Keith Packard

<#part sign=pgpmime>
Quoted 4 lines · Dave Jones
i915_drm_thaw is a deep nest of functions though, so this is going to be hard to track down where that write is coming from. Because the corruption seems to happen to pages that are already allocated, we probably can't even rely on DEBUG_PAGEALLOC, though it might be worth trying.

Quoted 5 lines

I'm worried that the write is coming through the GTT, which would make sense as these look like pixel values. If this is on Ironlake (core I3-I7 first gen), we know there are issues when VT-d is enabled, and the work-around for that doesn't appear to be in place for the hibernate resume case.

Thinking about how the GTT could contain stale pointers, I came up with this scenario:

Before we begin the thaw, the initramfs sets up a framebuffer. This causes the GTT to be setup.

- Thaw begins, hardware state still points to the GTT setup by the modesetting code. At this point, any graphics operations are going to cause writes through those translations. Bad news if we just wrote a bunch of thawed data there.

or..

- Thaw begins, and data is written over the GTT setup by the initramfs, but the hardware registers still points at it, until thaw is complete, when we reprogram the GTT registers to their pre-hibernate values.

If we could somehow set modeset=0 automatically if we detect a hibernate partition it would probably 'solve' it, but I suspect the real answer would be to do GTT teardown before we do a thaw.

Dave

Keith Packard

2012-03-13 00:06 UTC #15 lore raw

<#part sign=pgpmime>

Quoted 4 lines · Dave Jones

Thinking about how the GTT could contain stale pointers, I came up with this scenario:
Before we begin the thaw, the initramfs sets up a framebuffer. This causes the GTT to be setup.

Yes. The frame buffer is allocated as regular kernel pages, of course.

Quoted 4 lines

- Thaw begins, hardware state still points to the GTT setup by the modesetting code. At this point, any graphics operations are going to cause writes through those translations. Bad news if we just wrote a bunch of thawed data there.

The question is what data could still be pending there; we're running just fbcon at that point, which uses only write-through access. Presumably, any fbdev writes will have been long-since finished once we start the new kernel.

Quoted 3 lines

- Thaw begins, and data is written over the GTT setup by the initramfs, but the hardware registers still points at it, until thaw is complete, when we reprogram the GTT registers to their pre-hibernate values.

I'm not sure how the new kernel could manage to do any writes through this though -- it shouldn't touch the frame buffer until it has thawed the video driver, right?

Quoted 3 lines

If we could somehow set modeset=0 automatically if we detect a hibernate partition it would probably 'solve' it, but I suspect the real answer would be to do GTT teardown before we do a thaw.

We've got a ton of memory available in the 'stolen' area which the BIOS used as a frame buffer; we should be able to switch to that before the switch, if we decide that this is necessary.

Signature · 1 line

[email protected]