[PATCH 1/2] trace_uprobe: Use %lx to display offset

[PATCH 1/2] trace_uprobe: Use %lx to display offset

[Ravi Bangoria]

Kees suggested that, 'offset' is unsigned long, not a pointer,
thus %lx should be used to print it, not the %px. Also, I don't
see any reason to prepend offset with 0s. Replace %px with %lx.

Before patch:
  # echo "p:probe_a/main /tmp/a.out:0x594" > uprobe_events
  # cat uprobe_events
    p:probe_a/main /tmp/a.out:0x0000000000000594

After patch:
  # echo "p:probe_a/main /tmp/a.out:0x594" > uprobe_events
  # cat uprobe_events
    p:probe_a/main /tmp/a.out:0x594

Fixes: 0e4d819d0893 ("trace_uprobe: Display correct offset in uprobe_events")
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 kernel/trace/trace_uprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 268029ae1be6..c2c965398893 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -608,7 +608,7 @@ static int probes_seq_show(struct seq_file *m, void *v)
 
 	/* Don't print "0x  (null)" when offset is 0 */
 	if (tu->offset) {
-		seq_printf(m, "0x%px", (void *)tu->offset);
+		seq_printf(m, "0x%lx", tu->offset);
 	} else {
 		switch (sizeof(void *)) {
 		case 4:
-- 
2.13.6

[PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Ravi Bangoria]

Simplify probes_seq_show() function. We are using %lx to display
the offset and we don't prepend unnecessary 0s in the offset.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 kernel/trace/trace_uprobe.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c2c965398893..c12a3957e1ee 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -602,24 +602,9 @@ static int probes_seq_show(struct seq_file *m, void *v)
 	char c = is_ret_probe(tu) ? 'r' : 'p';
 	int i;
 
-	seq_printf(m, "%c:%s/%s", c, tu->tp.call.class->system,
-			trace_event_name(&tu->tp.call));
-	seq_printf(m, " %s:", tu->filename);
-
-	/* Don't print "0x  (null)" when offset is 0 */
-	if (tu->offset) {
-		seq_printf(m, "0x%lx", tu->offset);
-	} else {
-		switch (sizeof(void *)) {
-		case 4:
-			seq_printf(m, "0x00000000");
-			break;
-		case 8:
-		default:
-			seq_printf(m, "0x0000000000000000");
-			break;
-		}
-	}
+	seq_printf(m, "%c:%s/%s %s:0x%lx", c, tu->tp.call.class->system,
+			trace_event_name(&tu->tp.call), tu->filename,
+			tu->offset);
 
 	for (i = 0; i < tu->tp.nr_args; i++)
 		seq_printf(m, " %s=%s", tu->tp.args[i].name, tu->tp.args[i].comm);
-- 
2.13.6

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Srikar Dronamraju]

> Simplify probes_seq_show() function. We are using %lx to display
> the offset and we don't prepend unnecessary 0s in the offset.
> 


The prepending of 0s was introduced by 
tracing/uprobes: Do not print '0x (null)' when offset is 0
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a2fb3382

Wan, 
Was there a reason to prepend 0s?

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Kees Cook]

On Tue, Feb 6, 2018 at 8:34 PM, Ravi Bangoria
<ravi.bangoria@linux.vnet.ibm.com> wrote:
> Simplify probes_seq_show() function. We are using %lx to display
> the offset and we don't prepend unnecessary 0s in the offset.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_uprobe.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c2c965398893..c12a3957e1ee 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -602,24 +602,9 @@ static int probes_seq_show(struct seq_file *m, void *v)
>         char c = is_ret_probe(tu) ? 'r' : 'p';
>         int i;
>
> -       seq_printf(m, "%c:%s/%s", c, tu->tp.call.class->system,
> -                       trace_event_name(&tu->tp.call));
> -       seq_printf(m, " %s:", tu->filename);
> -
> -       /* Don't print "0x  (null)" when offset is 0 */
> -       if (tu->offset) {
> -               seq_printf(m, "0x%lx", tu->offset);
> -       } else {
> -               switch (sizeof(void *)) {
> -               case 4:
> -                       seq_printf(m, "0x00000000");
> -                       break;
> -               case 8:
> -               default:
> -                       seq_printf(m, "0x0000000000000000");
> -                       break;
> -               }
> -       }
> +       seq_printf(m, "%c:%s/%s %s:0x%lx", c, tu->tp.call.class->system,
> +                       trace_event_name(&tu->tp.call), tu->filename,
> +                       tu->offset);

To keep the prepended zeros (and avoid the redundant 0x prefix):

"...%#0*lx...", ... sizeof(void *) * 2, tu->offset);

As in:

+       seq_printf(m, "%c:%s/%s %s:%#0*lx", c, tu->tp.call.class->system,
+                       trace_event_name(&tu->tp.call), tu->filename,
+                       sizeof(void *) * 2, tu->offset);

-Kees

>
>         for (i = 0; i < tu->tp.nr_args; i++)
>                 seq_printf(m, " %s=%s", tu->tp.args[i].name, tu->tp.args[i].comm);
> --
> 2.13.6
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Ravi Bangoria]

On 02/08/2018 08:59 AM, Kees Cook wrote:
> On Tue, Feb 6, 2018 at 8:34 PM, Ravi Bangoria
> <ravi.bangoria@linux.vnet.ibm.com> wrote:
>> Simplify probes_seq_show() function. We are using %lx to display
>> the offset and we don't prepend unnecessary 0s in the offset.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> ---
>>  kernel/trace/trace_uprobe.c | 21 +++------------------
>>  1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index c2c965398893..c12a3957e1ee 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -602,24 +602,9 @@ static int probes_seq_show(struct seq_file *m, void *v)
>>         char c = is_ret_probe(tu) ? 'r' : 'p';
>>         int i;
>>
>> -       seq_printf(m, "%c:%s/%s", c, tu->tp.call.class->system,
>> -                       trace_event_name(&tu->tp.call));
>> -       seq_printf(m, " %s:", tu->filename);
>> -
>> -       /* Don't print "0x  (null)" when offset is 0 */
>> -       if (tu->offset) {
>> -               seq_printf(m, "0x%lx", tu->offset);
>> -       } else {
>> -               switch (sizeof(void *)) {
>> -               case 4:
>> -                       seq_printf(m, "0x00000000");
>> -                       break;
>> -               case 8:
>> -               default:
>> -                       seq_printf(m, "0x0000000000000000");
>> -                       break;
>> -               }
>> -       }
>> +       seq_printf(m, "%c:%s/%s %s:0x%lx", c, tu->tp.call.class->system,
>> +                       trace_event_name(&tu->tp.call), tu->filename,
>> +                       tu->offset);
> To keep the prepended zeros (and avoid the redundant 0x prefix):
>
> "...%#0*lx...", ... sizeof(void *) * 2, tu->offset);
>
> As in:
>
> +       seq_printf(m, "%c:%s/%s %s:%#0*lx", c, tu->tp.call.class->system,
> +                       trace_event_name(&tu->tp.call), tu->filename,
> +                       sizeof(void *) * 2, tu->offset);

This is useful, thanks Kees.

@Wang, Do we really need those 0s? Won't just 0x0 should
suffice? Here is the sample output...

  # echo "p:probe_a/main /tmp/a.out:0x0" > uprobe_events

Before patch:
  # cat uprobe_events
    p:probe_a/main /tmp/a.out:0x0000000000000000

After patch:
  # cat uprobe_events
    p:probe_a/main /tmp/a.out:0x0

Thanks,
Ravi

> -Kees
>
>>         for (i = 0; i < tu->tp.nr_args; i++)
>>                 seq_printf(m, " %s=%s", tu->tp.args[i].name, tu->tp.args[i].comm);
>> --
>> 2.13.6
>>
>
>

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Ravi Bangoria]

On 02/08/2018 09:13 AM, Ravi Bangoria wrote:
>
> On 02/08/2018 08:59 AM, Kees Cook wrote:
>> On Tue, Feb 6, 2018 at 8:34 PM, Ravi Bangoria
>> <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>> Simplify probes_seq_show() function. We are using %lx to display
>>> the offset and we don't prepend unnecessary 0s in the offset.
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>>> ---
>>>  kernel/trace/trace_uprobe.c | 21 +++------------------
>>>  1 file changed, 3 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>>> index c2c965398893..c12a3957e1ee 100644
>>> --- a/kernel/trace/trace_uprobe.c
>>> +++ b/kernel/trace/trace_uprobe.c
>>> @@ -602,24 +602,9 @@ static int probes_seq_show(struct seq_file *m, void *v)
>>>         char c = is_ret_probe(tu) ? 'r' : 'p';
>>>         int i;
>>>
>>> -       seq_printf(m, "%c:%s/%s", c, tu->tp.call.class->system,
>>> -                       trace_event_name(&tu->tp.call));
>>> -       seq_printf(m, " %s:", tu->filename);
>>> -
>>> -       /* Don't print "0x  (null)" when offset is 0 */
>>> -       if (tu->offset) {
>>> -               seq_printf(m, "0x%lx", tu->offset);
>>> -       } else {
>>> -               switch (sizeof(void *)) {
>>> -               case 4:
>>> -                       seq_printf(m, "0x00000000");
>>> -                       break;
>>> -               case 8:
>>> -               default:
>>> -                       seq_printf(m, "0x0000000000000000");
>>> -                       break;
>>> -               }
>>> -       }
>>> +       seq_printf(m, "%c:%s/%s %s:0x%lx", c, tu->tp.call.class->system,
>>> +                       trace_event_name(&tu->tp.call), tu->filename,
>>> +                       tu->offset);
>> To keep the prepended zeros (and avoid the redundant 0x prefix):
>>
>> "...%#0*lx...", ... sizeof(void *) * 2, tu->offset);
>>
>> As in:
>>
>> +       seq_printf(m, "%c:%s/%s %s:%#0*lx", c, tu->tp.call.class->system,
>> +                       trace_event_name(&tu->tp.call), tu->filename,
>> +                       sizeof(void *) * 2, tu->offset);
> This is useful, thanks Kees.
>
> @Wang, Do we really need those 0s? Won't just 0x0 should
> suffice? Here is the sample output...
>
>   # echo "p:probe_a/main /tmp/a.out:0x0" > uprobe_events
>
> Before patch:
>   # cat uprobe_events
>     p:probe_a/main /tmp/a.out:0x0000000000000000
>
> After patch:
>   # cat uprobe_events
>     p:probe_a/main /tmp/a.out:0x0

Wang, ping :)

Kees, I don't hear back from Wang and no one has reported any issues with
the patches yet. Can I have your Acked-by?

Thanks,
Ravi

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Kees Cook]

On Tue, Mar 6, 2018 at 12:12 AM, Ravi Bangoria
<ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>
> On 02/08/2018 09:13 AM, Ravi Bangoria wrote:
>>
>> On 02/08/2018 08:59 AM, Kees Cook wrote:
>>> On Tue, Feb 6, 2018 at 8:34 PM, Ravi Bangoria
>>> <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>>> Simplify probes_seq_show() function. We are using %lx to display
>>>> the offset and we don't prepend unnecessary 0s in the offset.
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>>>> ---
>>>>  kernel/trace/trace_uprobe.c | 21 +++------------------
>>>>  1 file changed, 3 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>>>> index c2c965398893..c12a3957e1ee 100644
>>>> --- a/kernel/trace/trace_uprobe.c
>>>> +++ b/kernel/trace/trace_uprobe.c
>>>> @@ -602,24 +602,9 @@ static int probes_seq_show(struct seq_file *m, void *v)
>>>>         char c = is_ret_probe(tu) ? 'r' : 'p';
>>>>         int i;
>>>>
>>>> -       seq_printf(m, "%c:%s/%s", c, tu->tp.call.class->system,
>>>> -                       trace_event_name(&tu->tp.call));
>>>> -       seq_printf(m, " %s:", tu->filename);
>>>> -
>>>> -       /* Don't print "0x  (null)" when offset is 0 */
>>>> -       if (tu->offset) {
>>>> -               seq_printf(m, "0x%lx", tu->offset);
>>>> -       } else {
>>>> -               switch (sizeof(void *)) {
>>>> -               case 4:
>>>> -                       seq_printf(m, "0x00000000");
>>>> -                       break;
>>>> -               case 8:
>>>> -               default:
>>>> -                       seq_printf(m, "0x0000000000000000");
>>>> -                       break;
>>>> -               }
>>>> -       }
>>>> +       seq_printf(m, "%c:%s/%s %s:0x%lx", c, tu->tp.call.class->system,
>>>> +                       trace_event_name(&tu->tp.call), tu->filename,
>>>> +                       tu->offset);
>>> To keep the prepended zeros (and avoid the redundant 0x prefix):
>>>
>>> "...%#0*lx...", ... sizeof(void *) * 2, tu->offset);
>>>
>>> As in:
>>>
>>> +       seq_printf(m, "%c:%s/%s %s:%#0*lx", c, tu->tp.call.class->system,
>>> +                       trace_event_name(&tu->tp.call), tu->filename,
>>> +                       sizeof(void *) * 2, tu->offset);
>> This is useful, thanks Kees.
>>
>> @Wang, Do we really need those 0s? Won't just 0x0 should
>> suffice? Here is the sample output...
>>
>>   # echo "p:probe_a/main /tmp/a.out:0x0" > uprobe_events
>>
>> Before patch:
>>   # cat uprobe_events
>>     p:probe_a/main /tmp/a.out:0x0000000000000000
>>
>> After patch:
>>   # cat uprobe_events
>>     p:probe_a/main /tmp/a.out:0x0
>
> Wang, ping :)
>
> Kees, I don't hear back from Wang and no one has reported any issues with
> the patches yet. Can I have your Acked-by?

I didn't see a v2 of these patches with the output fixed?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Ravi Bangoria]

On 03/07/2018 03:34 AM, Kees Cook wrote:
> On Tue, Mar 6, 2018 at 12:12 AM, Ravi Bangoria
> <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>
>> On 02/08/2018 09:13 AM, Ravi Bangoria wrote:
>>> Wang, ping :)
>>>
>>> Kees, I don't hear back from Wang and no one has reported any issues with
>>> the patches yet. Can I have your Acked-by?
> I didn't see a v2 of these patches with the output fixed?

Kees, I didn't send v2 because those 0s seems unnecessary to me.

Please let me know if you still wants to show them. Will re-spin :)

Thanks,
Ravi

PS: Changing Masami's email address in cc.

Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()

[Kees Cook]

On Tue, Mar 6, 2018 at 8:12 PM, Ravi Bangoria
<ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>
> On 03/07/2018 03:34 AM, Kees Cook wrote:
>> On Tue, Mar 6, 2018 at 12:12 AM, Ravi Bangoria
>> <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>>
>>> On 02/08/2018 09:13 AM, Ravi Bangoria wrote:
>>>> Wang, ping :)
>>>>
>>>> Kees, I don't hear back from Wang and no one has reported any issues with
>>>> the patches yet. Can I have your Acked-by?
>> I didn't see a v2 of these patches with the output fixed?
>
> Kees, I didn't send v2 because those 0s seems unnecessary to me.

Oh, okay. I don't care either way. That's up to whoever consumes that
output. Usually it's bad form to change formatting for a userspace
API, but if nothing cares, then sure. :)

> Please let me know if you still wants to show them. Will re-spin :)

AIUI, it's "safer" to include them since that's what it did before.

-Kees

>
> Thanks,
> Ravi
>
> PS: Changing Masami's email address in cc.
>



-- 
Kees Cook
Pixel Security