fix oops in ext4_mb_release_group_pa tracing

PATCH In review Eric Sandeen 3 participants 5 messages Aug 19, 2010 – Oct 9, 2010

Subsystems (2)

Tracing ext4 file system

lore

Share & tools

lore

Eric Sandeen

author

2010-08-19 17:59 UTC #0 lore raw

PATCH 1/1 fix oops in ext4_mb_release_group_pa tracing

Our QA reported an oops in the ext4_mb_release_group_pa tracing, and Josef Bacik pointed out that it was because we may have a non-null but uninitialized ac_inode in the allocation context.

I can reproduce it when running xfstests with ext4 tracepoints on, on a CONFIG_SLAB_DEBUG kernel.

We call trace_ext4_mb_release_group_pa from 2 places, ext4_mb_discard_group_preallocations and ext4_mb_discard_lg_preallocations

In both cases we allocate an ac as a container just for tracing (!) and never fill in the ac_inode. There's no reason to be assigning, testing, or printing it as far as I can see, so just remove it from the tracepoint.

Signed-off-by Eric Sandeen <[email protected]>

---

include/trace/events/ext4.h +0 -3

index 01e9e00..e352c77 100644
@@ -432,7 +432,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)
-		__field(	ino_t,	ino			)
 		__field(	__u64,	pa_pstart		)
 		__field(	__u32,	pa_len			)

@@ -440,8 +439,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_fast_assign(
 		__entry->dev		= sb->s_dev;
-		__entry->ino		= (ac && ac->ac_inode) ?
-						ac->ac_inode->i_ino : 0;
 		__entry->pa_pstart	= pa->pa_pstart;
 		__entry->pa_len		= pa->pa_len;
 	),

Josef Bacik

Reviewed-by on v1 1/1

2010-08-19 18:02 UTC #1 lore raw

Quoted 33 lines · Eric Sandeen

Our QA reported an oops in the ext4_mb_release_group_pa tracing, and Josef Bacik pointed out that it was because we may have a non-null but uninitialized ac_inode in the allocation context.
I can reproduce it when running xfstests with ext4 tracepoints on, on a CONFIG_SLAB_DEBUG kernel.
We call trace_ext4_mb_release_group_pa from 2 places, ext4_mb_discard_group_preallocations and ext4_mb_discard_lg_preallocations
In both cases we allocate an ac as a container just for tracing (!) and never fill in the ac_inode. There's no reason to be assigning, testing, or printing it as far as I can see, so just remove it from the tracepoint.
Signed-off-by Eric Sandeen <[email protected]>
---
include/trace/events/ext4.h +0 -1
index 01e9e00..e352c77 100644
@@ -432,7 +432,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)
-		__field(	ino_t,	ino			)
 		__field(	__u64,	pa_pstart		)
 		__field(	__u32,	pa_len			)
 

include/trace/events/ext4.h TRACE_EVENT(ext4_mb_release_group_pa, · line 444

Quoted 8 lines

include/trace/events/ext4.h +0 -2

@@ -440,8 +439,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_fast_assign(
 		__entry->dev		= sb->s_dev;
-		__entry->ino		= (ac && ac->ac_inode) ?
-						ac->ac_inode->i_ino : 0;
 		__entry->pa_pstart	= pa->pa_pstart;
 		__entry->pa_len		= pa->pa_len;
 	),

Reviewed-by Josef Bacik <[email protected]>

Thanks,

Josef

Eric Sandeen

author

2010-10-07 16:45 UTC #2 lore raw

In reply to Eric Sandeen

Quoted 7 lines · Eric Sandeen

Our QA reported an oops in the ext4_mb_release_group_pa tracing, and Josef Bacik pointed out that it was because we may have a non-null but uninitialized ac_inode in the allocation context.
I can reproduce it when running xfstests with ext4 tracepoints on, on a CONFIG_SLAB_DEBUG kernel.

Quoted 8 lines

We call trace_ext4_mb_release_group_pa from 2 places, ext4_mb_discard_group_preallocations and ext4_mb_discard_lg_preallocations
In both cases we allocate an ac as a container just for tracing (!) and never fill in the ac_inode. There's no reason to be assigning, testing, or printing it as far as I can see, so just remove it from the tracepoint.

Ping on this one at least?

avoid-null-dereference-in-trace_ext4_mballoc_discard is in the patch queue, this one is similar and should go with it.

I'd really like to see
[PATCH] don't use ext4_allocation_contexts for tracing go soon as well so we stop abusing the slab cache all day long...

Thanks,
-Eric

Quoted 29 lines

Signed-off-by Eric Sandeen <[email protected]>

---

include/trace/events/ext4.h +0 -4

index 01e9e00..e352c77 100644
@@ -432,7 +432,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)
-		__field(	ino_t,	ino			)
 		__field(	__u64,	pa_pstart		)
 		__field(	__u32,	pa_len			)

@@ -440,8 +439,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_fast_assign(
 		__entry->dev		= sb->s_dev;
-		__entry->ino		= (ac && ac->ac_inode) ?
-						ac->ac_inode->i_ino : 0;
 		__entry->pa_pstart	= pa->pa_pstart;
 		__entry->pa_len		= pa->pa_len;
 	),
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

author

2010-10-07 16:47 UTC #3 lore raw

In reply to Eric Sandeen

Quoted 11 lines · Eric Sandeen

Our QA reported an oops in the ext4_mb_release_group_pa tracing, and Josef Bacik pointed out that it was because we may have a non-null but uninitialized ac_inode in the allocation context.
I can reproduce it when running xfstests with ext4 tracepoints on, on a CONFIG_SLAB_DEBUG kernel.
We call trace_ext4_mb_release_group_pa from 2 places, ext4_mb_discard_group_preallocations and ext4_mb_discard_lg_preallocations

Quoted 6 lines

In both cases we allocate an ac as a container just for tracing (!) and never fill in the ac_inode. There's no reason to be assigning, testing, or printing it as far as I can see, so just remove it from the tracepoint.
Signed-off-by Eric Sandeen <[email protected]>

ping on this one too, oopsing while tracing is bad ... ;)

-Eric

Quoted 28 lines

---

include/trace/events/ext4.h +0 -4

index 01e9e00..e352c77 100644
@@ -432,7 +432,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_STRUCT__entry(
 		__field(	dev_t,	dev			)
-		__field(	ino_t,	ino			)
 		__field(	__u64,	pa_pstart		)
 		__field(	__u32,	pa_len			)

@@ -440,8 +439,6 @@ TRACE_EVENT(ext4_mb_release_group_pa,
 
 	TP_fast_assign(
 		__entry->dev		= sb->s_dev;
-		__entry->ino		= (ac && ac->ac_inode) ?
-						ac->ac_inode->i_ino : 0;
 		__entry->pa_pstart	= pa->pa_pstart;
 		__entry->pa_len		= pa->pa_len;
 	),
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ted Ts'o

maintainer

2010-10-09 22:26 UTC #4 lore raw

In reply to Eric Sandeen

Quoted 11 lines · Eric Sandeen

Our QA reported an oops in the ext4_mb_release_group_pa tracing, and Josef Bacik pointed out that it was because we may have a non-null but uninitialized ac_inode in the allocation context.
I can reproduce it when running xfstests with ext4 tracepoints on, on a CONFIG_SLAB_DEBUG kernel.
We call trace_ext4_mb_release_group_pa from 2 places, ext4_mb_discard_group_preallocations and ext4_mb_discard_lg_preallocations

Quoted 6 lines

In both cases we allocate an ac as a container just for tracing (!) and never fill in the ac_inode. There's no reason to be assigning, testing, or printing it as far as I can see, so just remove it from the tracepoint.
Signed-off-by Eric Sandeen <[email protected]>

Thanks for the ping; I've added it to the ext4 patch tree.

- Ted