Messages in this thread | | | Date | Wed, 08 Oct 2008 13:04:16 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH] markers: fix unchecked format |
| |
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.
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. >> >> >
| |