Messages in this thread | | | Date | Thu, 9 Oct 2008 09:46:06 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH] markers: fix unchecked format |
| |
* Lai Jiangshan (laijs@cn.fujitsu.com) wrote: > > No. > > 1) > In current code, when the second, third... probe is registered > with the same marker name, its format is not checked. > > marker_probe_register("marker_name", "field1 %s", XXX); > marker_probe_register("marker_name", "field1 %d", XXX); > > the second call, "field1 %d" is not check for ever. > and this probe may cause kernel core-dump. > > because these two probes share the same marker_entry, and > we do not check the format when they are being shared. > > if several probes share the same marker_entry we should > make sure all these probes's format are the same. >
Yep, you are right. Thanks for the explanation.
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> 2) > set_marker() check marker's format with marker_entry's format > my fix change marker_probe_register(), > and marker_probe_register() check probes' format with marker_entry's format. > > they are not duplicate check. > > 3) > my patch change marker_probe_register(), and this fix can not > make the module load fail in an condition. > for: marker_update_probe_range() return void. > > Thanks, Lai. > > Mathieu Desnoyers wrote: > > * Lai Jiangshan (laijs@cn.fujitsu.com) wrote: > >> when the second, third... probe is registered, its format is > >> not checked, this patch fix it. > >> > > > > It's already checked here : > > > > marker_update_probes > > marker_update_probe_range > > set_marker > > > > if ((*entry)->format) { > > if (strcmp((*entry)->format, elem->format) != 0) { > > printk(KERN_NOTICE > > "Format mismatch for probe %s " > > "(%s), marker (%s)\n", > > (*entry)->name, > > (*entry)->format, > > elem->format); > > return -EPERM; > > } > > } else { > > ret = marker_set_format(entry, elem->format); > > if (ret) > > return ret; > > } > > > > Given that marker_probe_register can be called to connect a probe to a > > marker which does not exist yet (e.g. marker in a module not loaded), I > > am not sure it makes sense to check for format string mismatch so early > > in marker_probe_register (the moment it adds the marker to the hash > > table). That's actually why I chose to leave it in later stage which > > does the actual connection of the probes to the markers > > (marker_update_probes). > > > > If you really want to check it earlier, how do you plan to deal with > > this scenario ? > > > > 1 - a marker probe is registered for markerA with format string > > "field1 %s" > > 2 - a module is loaded, which contains a marker markerA with format > > string "field1 %d" > > > > I think it would be _really_ bad to make the module load fail because of > > a marker format string mismatch... this is why I chose just to give a > > warning in set_marker, which is shown when the markers are updated, > > which happens when the module is loaded and when the marker hash table > > is modified. > > > > Mathieu > > > >> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> > >> --- > >> diff --git a/kernel/marker.c b/kernel/marker.c > >> index 4440a09..1196a6b 100644 > >> --- a/kernel/marker.c > >> +++ b/kernel/marker.c > >> @@ -651,11 +651,17 @@ int marker_probe_register(const char *name, const char *format, > >> entry = get_marker(name); > >> if (!entry) { > >> entry = add_marker(name, format); > >> - if (IS_ERR(entry)) { > >> + if (IS_ERR(entry)) > >> ret = PTR_ERR(entry); > >> - goto end; > >> - } > >> + } else if (format) { > >> + if (!entry->format) > >> + ret = marker_set_format(&entry, format); > >> + else if (strcmp(entry->format, format)) > >> + ret = -EPERM; > >> } > >> + if (ret) > >> + goto end; > >> + > >> /* > >> * If we detect that a call_rcu is pending for this marker, > >> * make sure it's executed now. > >> > >> > > > >
-- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
| |