markers: fix unchecked format

when the second, third... probe is registered, its format is not checked, this patch fix it.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/marker.c +9 -3

index 4440a09..1196a6b 100644
@@ -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.

Quoted 3 lines · * Lai Jiangshan ([email protected]) 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

Quoted 29 lines

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/marker.c +9 -3

index 4440a09..1196a6b 100644
@@ -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.

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.

Quoted 81 lines · Mathieu Desnoyers

Quoted 3 lines · * Lai Jiangshan ([email protected]) 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

Quoted 29 lines

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/marker.c +9 -3

index 4440a09..1196a6b 100644
@@ -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.

Quoted 16 lines · * Lai Jiangshan ([email protected]) 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.

Quoted 3 lines

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 <[email protected]>