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]>
---
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;
),
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 <[email protected]>
> ---
>
> 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 <[email protected]>
Thanks,
Josef
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 <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
Thanks for the ping; I've added it to the ext4 patch tree.
- Ted