[PATCH] fix oops in ext4_mb_release_group_pa tracing

[PATCH] fix oops in ext4_mb_release_group_pa tracing

[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 <sandeen@redhat.com>
---

diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 01e9e00..e352c77 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -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;
 	),

Re: [PATCH] fix oops in ext4_mb_release_group_pa tracing

[Josef Bacik]

On Thu, Aug 19, 2010 at 12:59:50PM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
> 
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index 01e9e00..e352c77 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -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;
>  	),
> 

Reviewed-by: Josef Bacik <josef@redhat.com>

Thanks,

Josef

Re: [PATCH] fix oops in ext4_mb_release_group_pa tracing

[Eric Sandeen]

On 08/19/2010 12:59 PM, Eric Sandeen wrote:
> 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.

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

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index 01e9e00..e352c77 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix oops in ext4_mb_release_group_pa tracing

[Eric Sandeen]

On 08/19/2010 12:59 PM, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>

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

-Eric

> ---
> 
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index 01e9e00..e352c77 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix oops in ext4_mb_release_group_pa tracing

[Ted Ts'o]

On Thu, Aug 19, 2010 at 12:59:50PM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>

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

       	       	     	  	      - Ted