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 <[email protected]>
Signed-off-by: Ravi Bangoria <[email protected]>
---
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
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 <[email protected]>
---
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
> 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?
On Tue, Feb 6, 2018 at 8:34 PM, Ravi Bangoria
<[email protected]> 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 <[email protected]>
> ---
> 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
On 02/08/2018 08:59 AM, Kees Cook wrote:
> On Tue, Feb 6, 2018 at 8:34 PM, Ravi Bangoria
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
>
>
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
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
On Tue, Mar 6, 2018 at 12:12 AM, Ravi Bangoria
<[email protected]> 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
>>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
On 03/07/2018 03:34 AM, Kees Cook wrote:
> On Tue, Mar 6, 2018 at 12:12 AM, Ravi Bangoria
> <[email protected]> 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.
On Tue, Mar 6, 2018 at 8:12 PM, Ravi Bangoria
<[email protected]> wrote:
>
>
> On 03/07/2018 03:34 AM, Kees Cook wrote:
>> On Tue, Mar 6, 2018 at 12:12 AM, Ravi Bangoria
>> <[email protected]> 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