* [PATCH 1/2] trace_uprobe: Use %lx to display offset
@ 2018-02-06 9:34 Ravi Bangoria
2018-02-06 9:34 ` [PATCH 2/2] trace_uprobe: Simplify probes_seq_show() Ravi Bangoria
0 siblings, 1 reply; 9+ messages in thread
From: Ravi Bangoria @ 2018-02-06 9:34 UTC (permalink / raw)
To: rostedt
Cc: mingo, srikar, oleg, wangnan0, masami.hiramatsu.pt, keescook,
namhyung, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-02-06 9:34 [PATCH 1/2] trace_uprobe: Use %lx to display offset Ravi Bangoria
@ 2018-02-06 9:34 ` Ravi Bangoria
2018-02-06 12:47 ` Srikar Dronamraju
2018-02-08 3:29 ` Kees Cook
0 siblings, 2 replies; 9+ messages in thread
From: Ravi Bangoria @ 2018-02-06 9:34 UTC (permalink / raw)
To: rostedt
Cc: mingo, srikar, oleg, wangnan0, masami.hiramatsu.pt, keescook,
namhyung, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-02-06 9:34 ` [PATCH 2/2] trace_uprobe: Simplify probes_seq_show() Ravi Bangoria
@ 2018-02-06 12:47 ` Srikar Dronamraju
2018-02-08 3:29 ` Kees Cook
1 sibling, 0 replies; 9+ messages in thread
From: Srikar Dronamraju @ 2018-02-06 12:47 UTC (permalink / raw)
To: Ravi Bangoria
Cc: rostedt, mingo, oleg, wangnan0, masami.hiramatsu.pt, keescook,
namhyung, linux-kernel
> 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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-02-06 9:34 ` [PATCH 2/2] trace_uprobe: Simplify probes_seq_show() Ravi Bangoria
2018-02-06 12:47 ` Srikar Dronamraju
@ 2018-02-08 3:29 ` Kees Cook
2018-02-08 3:43 ` Ravi Bangoria
1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2018-02-08 3:29 UTC (permalink / raw)
To: Ravi Bangoria
Cc: Steven Rostedt, Ingo Molnar, Srikar Dronamraju, Oleg Nesterov,
wangnan0, Masami Hiramatsu, Namhyung Kim, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-02-08 3:29 ` Kees Cook
@ 2018-02-08 3:43 ` Ravi Bangoria
2018-03-06 8:12 ` Ravi Bangoria
0 siblings, 1 reply; 9+ messages in thread
From: Ravi Bangoria @ 2018-02-08 3:43 UTC (permalink / raw)
To: Kees Cook, wangnan0
Cc: Steven Rostedt, Ingo Molnar, Srikar Dronamraju, Oleg Nesterov,
Masami Hiramatsu, Namhyung Kim, LKML, 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
>>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-02-08 3:43 ` Ravi Bangoria
@ 2018-03-06 8:12 ` Ravi Bangoria
2018-03-06 22:04 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Ravi Bangoria @ 2018-03-06 8:12 UTC (permalink / raw)
To: Kees Cook, wangnan0
Cc: Steven Rostedt, Ingo Molnar, Srikar Dronamraju, Oleg Nesterov,
Masami Hiramatsu, Namhyung Kim, LKML, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-03-06 8:12 ` Ravi Bangoria
@ 2018-03-06 22:04 ` Kees Cook
2018-03-07 4:12 ` Ravi Bangoria
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2018-03-06 22:04 UTC (permalink / raw)
To: Ravi Bangoria
Cc: wangnan0, Steven Rostedt, Ingo Molnar, Srikar Dronamraju,
Oleg Nesterov, Masami Hiramatsu, Namhyung Kim, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-03-06 22:04 ` Kees Cook
@ 2018-03-07 4:12 ` Ravi Bangoria
2018-03-07 22:17 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Ravi Bangoria @ 2018-03-07 4:12 UTC (permalink / raw)
To: Kees Cook
Cc: wangnan0, Steven Rostedt, Ingo Molnar, Srikar Dronamraju,
Oleg Nesterov, Masami Hiramatsu, Namhyung Kim, LKML,
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] trace_uprobe: Simplify probes_seq_show()
2018-03-07 4:12 ` Ravi Bangoria
@ 2018-03-07 22:17 ` Kees Cook
0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2018-03-07 22:17 UTC (permalink / raw)
To: Ravi Bangoria
Cc: wangnan0, Steven Rostedt, Ingo Molnar, Srikar Dronamraju,
Oleg Nesterov, Masami Hiramatsu, Namhyung Kim, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-03-07 22:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06 9:34 [PATCH 1/2] trace_uprobe: Use %lx to display offset Ravi Bangoria
2018-02-06 9:34 ` [PATCH 2/2] trace_uprobe: Simplify probes_seq_show() Ravi Bangoria
2018-02-06 12:47 ` Srikar Dronamraju
2018-02-08 3:29 ` Kees Cook
2018-02-08 3:43 ` Ravi Bangoria
2018-03-06 8:12 ` Ravi Bangoria
2018-03-06 22:04 ` Kees Cook
2018-03-07 4:12 ` Ravi Bangoria
2018-03-07 22:17 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).