: bug fix, remove partial zero out
in [Kernel]
|
Prev: [PATCH] mm: Fix nr_good_pages calculation
Next: sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync
From: Frederic Weisbecker on 20 Jan 2010 13:00 On Tue, Jan 19, 2010 at 03:33:56PM +0800, Lai Jiangshan wrote: > partial-zero-out a struct is very dangerous, we should zero out > field by field directly when need. > > partial-zero-out for struct trace_iterator exists when ftrace > was first introduced into mainline kernel. But in this few years, > the code of ftrace is changed a lot, and: > > 1) partial-zero-out for struct trace_iterator has a bug now, > cpumask_var_t started should not be zeroed out. > > 2) I viewed the codes and found that fields below > "/* The below is zeroed out in pipe_read */" > don't need to be zeroed out or initialized now. > > So, we remove the code of "partial zero out" > > Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com> > --- > diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h > index 3ca9485..c6d0e1a 100644 > --- a/include/linux/ftrace_event.h > +++ b/include/linux/ftrace_event.h > @@ -54,7 +54,6 @@ struct trace_iterator { > struct ring_buffer_iter *buffer_iter[NR_CPUS]; > unsigned long iter_flags; > > - /* The below is zeroed out in pipe_read */ > struct trace_seq seq; > struct trace_entry *ent; > int leftover; > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 5314c90..27fecf8 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -3124,12 +3124,6 @@ waitagain: > if (cnt >= PAGE_SIZE) > cnt = PAGE_SIZE - 1; > > - /* reset all but tr, trace, and overruns */ > - memset(&iter->seq, 0, > - sizeof(struct trace_iterator) - > - offsetof(struct trace_iterator, seq)); > - iter->pos = -1; > - I'm not sure exaclty why we needed to zero the seq here. We already reset it in trace_seq_init(). We might do it again on waitagain. I lost track how we could ever need to goto waitagain. It was about a tricky bug to fix but I'm don't remember exactly the details. That said, if trace_seq_to_user returns -EBUSY, we re-init the seq buffer, so it should be fine I guess. But concerning the need of setting iter->pos to -1, I'm not sure we need to remove it. Shouldn't it be set to 0 btw? Steve? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Lai Jiangshan on 25 Jan 2010 22:40 Frederic Weisbecker wrote: > On Tue, Jan 19, 2010 at 03:33:56PM +0800, Lai Jiangshan wrote: >> partial-zero-out a struct is very dangerous, we should zero out >> field by field directly when need. >> >> partial-zero-out for struct trace_iterator exists when ftrace >> was first introduced into mainline kernel. But in this few years, >> the code of ftrace is changed a lot, and: >> >> 1) partial-zero-out for struct trace_iterator has a bug now, >> cpumask_var_t started should not be zeroed out. >> >> 2) I viewed the codes and found that fields below >> "/* The below is zeroed out in pipe_read */" >> don't need to be zeroed out or initialized now. >> >> So, we remove the code of "partial zero out" >> >> Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com> >> --- >> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h >> index 3ca9485..c6d0e1a 100644 >> --- a/include/linux/ftrace_event.h >> +++ b/include/linux/ftrace_event.h >> @@ -54,7 +54,6 @@ struct trace_iterator { >> struct ring_buffer_iter *buffer_iter[NR_CPUS]; >> unsigned long iter_flags; >> >> - /* The below is zeroed out in pipe_read */ >> struct trace_seq seq; >> struct trace_entry *ent; >> int leftover; >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index 5314c90..27fecf8 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -3124,12 +3124,6 @@ waitagain: >> if (cnt >= PAGE_SIZE) >> cnt = PAGE_SIZE - 1; >> >> - /* reset all but tr, trace, and overruns */ >> - memset(&iter->seq, 0, >> - sizeof(struct trace_iterator) - >> - offsetof(struct trace_iterator, seq)); >> - iter->pos = -1; >> - > > > > I'm not sure exaclty why we needed to zero the seq here. > We already reset it in trace_seq_init(). > > We might do it again on waitagain. I lost track how we could > ever need to goto waitagain. It was about a tricky bug to fix > but I'm don't remember exactly the details. > > That said, if trace_seq_to_user returns -EBUSY, we > re-init the seq buffer, so it should be fine I guess. Yes, -EBUSY is strange here. but any way, trace_seq_init() is called. > > But concerning the need of setting iter->pos to -1, I'm not > sure we need to remove it. Shouldn't it be set to 0 btw? > ->pos is not used here, ->idx is just increased here, so we don't need to initialize them. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 30 Jan 2010 16:30
On Tue, Jan 26, 2010 at 11:31:58AM +0800, Lai Jiangshan wrote: > Frederic Weisbecker wrote: > > On Tue, Jan 19, 2010 at 03:33:56PM +0800, Lai Jiangshan wrote: > >> partial-zero-out a struct is very dangerous, we should zero out > >> field by field directly when need. > >> > >> partial-zero-out for struct trace_iterator exists when ftrace > >> was first introduced into mainline kernel. But in this few years, > >> the code of ftrace is changed a lot, and: > >> > >> 1) partial-zero-out for struct trace_iterator has a bug now, > >> cpumask_var_t started should not be zeroed out. > >> > >> 2) I viewed the codes and found that fields below > >> "/* The below is zeroed out in pipe_read */" > >> don't need to be zeroed out or initialized now. > >> > >> So, we remove the code of "partial zero out" > >> > >> Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com> > >> --- > >> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h > >> index 3ca9485..c6d0e1a 100644 > >> --- a/include/linux/ftrace_event.h > >> +++ b/include/linux/ftrace_event.h > >> @@ -54,7 +54,6 @@ struct trace_iterator { > >> struct ring_buffer_iter *buffer_iter[NR_CPUS]; > >> unsigned long iter_flags; > >> > >> - /* The below is zeroed out in pipe_read */ > >> struct trace_seq seq; > >> struct trace_entry *ent; > >> int leftover; > >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > >> index 5314c90..27fecf8 100644 > >> --- a/kernel/trace/trace.c > >> +++ b/kernel/trace/trace.c > >> @@ -3124,12 +3124,6 @@ waitagain: > >> if (cnt >= PAGE_SIZE) > >> cnt = PAGE_SIZE - 1; > >> > >> - /* reset all but tr, trace, and overruns */ > >> - memset(&iter->seq, 0, > >> - sizeof(struct trace_iterator) - > >> - offsetof(struct trace_iterator, seq)); > >> - iter->pos = -1; > >> - > > > > > > > > I'm not sure exaclty why we needed to zero the seq here. > > We already reset it in trace_seq_init(). > > > > We might do it again on waitagain. I lost track how we could > > ever need to goto waitagain. It was about a tricky bug to fix > > but I'm don't remember exactly the details. > > > > That said, if trace_seq_to_user returns -EBUSY, we > > re-init the seq buffer, so it should be fine I guess. > > Yes, -EBUSY is strange here. > but any way, trace_seq_init() is called. > > > > > But concerning the need of setting iter->pos to -1, I'm not > > sure we need to remove it. Shouldn't it be set to 0 btw? > > > > ->pos is not used here, ->idx is just increased here, > so we don't need to initialize them. Ok. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |