[PATCH] markers: fix unchecked format

[PATCH] markers: fix unchecked format

[Lai Jiangshan]

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

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.

Re: [PATCH] markers: fix unchecked format

[Mathieu Desnoyers]

* 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

Re: [PATCH] markers: fix unchecked format

[Lai Jiangshan]

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.
>>
>>
> 

Re: [PATCH] markers: fix unchecked format

[Mathieu Desnoyers]

* 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

Re: [PATCH] markers: fix unchecked format

[Ingo Molnar]

* Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> * 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>

applied to tip/tracing/markers, thanks!

	Ingo