diff mbox series

tracing/timerlat: Move hrtimer_init to timerlat_fd open()

Message ID 7324dd3fc0035658c99b825204a66049389c56e3.1706798888.git.bristot@kernel.org
State New
Headers
Series tracing/timerlat: Move hrtimer_init to timerlat_fd open() |

Commit Message

Daniel Bristot de Oliveira Feb. 1, 2024, 3:13 p.m. UTC 
  Currently, the timerlat's hrtimer is initialized at the first read of
timerlat_fd, and destroyed at close(). It works, but it causes an error
if the user program open() and close() the file without reading.

Move hrtimer_init to timerlat_fd open() to avoid this problem.

No functional changes.

Fixes: e88ed227f639 ("tracing/timerlat: Add user-space interface")
Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
---
 kernel/trace/trace_osnoise.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Daniel Bristot de Oliveira Feb. 1, 2024, 3:27 p.m. UTC | #1 
On 2/1/24 16:25, Steven Rostedt wrote:
> On Thu,  1 Feb 2024 16:13:39 +0100
> Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
> 
>> Currently, the timerlat's hrtimer is initialized at the first read of
>> timerlat_fd, and destroyed at close(). It works, but it causes an error
>> if the user program open() and close() the file without reading.
>>
>> Move hrtimer_init to timerlat_fd open() to avoid this problem.
>>
>> No functional changes.
> 
> It can't be fixing something and not have any functional changes.
> 
> No functional changes means the code is restructured but the resulting
> assembly would be the same.
> 
> Like moving functions around in a file so that you don't need extra
> prototype declarations.
> 
> Please only add "No functional changes" if the function's assembly would be
> the same.

ok

>>
>> Fixes: e88ed227f639 ("tracing/timerlat: Add user-space interface")
> 
> With a fixes tag, I'm assuming his should go into v6.8 with a Cc stable?

right, I added it on Cc, but did not include the Cc:.. tag. It seems that I should have.

-- Daniel

> -- Steve
> 
> 
>> Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
>> ---
>>  kernel/trace/trace_osnoise.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
>> index bd0d01d00fb9..a8e28f9b9271 100644
>> --- a/kernel/trace/trace_osnoise.c
>> +++ b/kernel/trace/trace_osnoise.c
>> @@ -2444,6 +2444,9 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
>>  	tlat = this_cpu_tmr_var();
>>  	tlat->count = 0;
>>  
>> +	hrtimer_init(&tlat->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
>> +	tlat->timer.function = timerlat_irq;
>> +
>>  	migrate_enable();
>>  	return 0;
>>  };
>> @@ -2526,9 +2529,6 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
>>  		tlat->tracing_thread = false;
>>  		tlat->kthread = current;
>>  
>> -		hrtimer_init(&tlat->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
>> -		tlat->timer.function = timerlat_irq;
>> -
>>  		/* Annotate now to drift new period */
>>  		tlat->abs_period = hrtimer_cb_get_time(&tlat->timer);
>>  
>
Greg KH Feb. 1, 2024, 3:44 p.m. UTC | #2 
On Thu, Feb 01, 2024 at 04:13:39PM +0100, Daniel Bristot de Oliveira wrote:
> Currently, the timerlat's hrtimer is initialized at the first read of
> timerlat_fd, and destroyed at close(). It works, but it causes an error
> if the user program open() and close() the file without reading.

What error exactly happens?  Userspace, or the kernel crashes?

thanks,

greg k-h
Daniel Bristot de Oliveira Feb. 1, 2024, 4:02 p.m. UTC | #3 
On 2/1/24 16:44, Greg KH wrote:
> On Thu, Feb 01, 2024 at 04:13:39PM +0100, Daniel Bristot de Oliveira wrote:
>> Currently, the timerlat's hrtimer is initialized at the first read of
>> timerlat_fd, and destroyed at close(). It works, but it causes an error
>> if the user program open() and close() the file without reading.
> 
> What error exactly happens?  Userspace, or the kernel crashes?

sorry, kernel crash:

# echo NO_OSNOISE_WORKLOAD > /sys/kernel/debug/tracing/osnoise/options
# echo timerlat > /sys/kernel/debug/tracing/current_tracer

# cat ./timerlat_load.py
#!/usr/bin/env python3

timerlat_fd = open("/sys/kernel/tracing/osnoise/per_cpu/cpu0/timerlat_fd", 'r')
timerlat_fd.close();

# ./taskset -c 0 ./timerlat_load.py
<BOOM>

[ 6401.611374] BUG: kernel NULL pointer dereference, address: 0000000000000010
[ 6401.611786] #PF: supervisor read access in kernel mode
[ 6401.612081] #PF: error_code(0x0000) - not-present page
[ 6401.612376] PGD 0 P4D 0
[ 6401.612495] Oops: 0000 [#1] PREEMPT SMP NOPTI
[ 6401.612690] CPU: 1 PID: 2673 Comm: python3 Not tainted 6.6.13-200.fc39.x86_64 #1
[ 6401.613011] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39 04/01/2014
[ 6401.613379] RIP: 0010:hrtimer_active+0xd/0x50
[ 6401.613577] Code: 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 48 8b 57 30 <8b> 42 10 a8 01 74 09 f3 90 8b 42 10 a8 01 75 f7 80 7f 38 00 75 1d
[ 6401.614374] RSP: 0018:ffffb031009b7e10 EFLAGS: 00010286
[ 6401.614604] RAX: 000000000002db00 RBX: ffff9118f786db08 RCX: 0000000000000000
[ 6401.614914] RDX: 0000000000000000 RSI: ffff9117a0e64400 RDI: ffff9118f786db08
[ 6401.615225] RBP: ffff9118f786db80 R08: ffff9117a0ddd420 R09: ffff9117804d4f70
[ 6401.615534] R10: 0000000000000000 R11: 0000000000000000 R12: ffff9118f786db08
[ 6401.615846] R13: ffff91178fdd5e20 R14: ffff9117840978c0 R15: 0000000000000000
[ 6401.616157] FS:  00007f2ffbab1740(0000) GS:ffff9118f7840000(0000) knlGS:0000000000000000
[ 6401.616508] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6401.616765] CR2: 0000000000000010 CR3: 00000001b402e000 CR4: 0000000000750ee0
[ 6401.617075] PKRU: 55555554
[ 6401.617197] Call Trace:
[ 6401.617309]  <TASK>
[ 6401.617407]  ? __die+0x23/0x70
[ 6401.617548]  ? page_fault_oops+0x171/0x4e0
[ 6401.617983]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.618389]  ? avc_has_extended_perms+0x237/0x520
[ 6401.618800]  ? exc_page_fault+0x7f/0x180
[ 6401.619176]  ? asm_exc_page_fault+0x26/0x30
[ 6401.619563]  ? hrtimer_active+0xd/0x50
[ 6401.619926]  hrtimer_cancel+0x15/0x40
[ 6401.620286]  timerlat_fd_release+0x48/0xe0
[ 6401.620666]  __fput+0xf5/0x290
[ 6401.621004]  __x64_sys_close+0x3d/0x80
[ 6401.621370]  do_syscall_64+0x60/0x90
[ 6401.621730]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.622129]  ? __x64_sys_ioctl+0x72/0xd0
[ 6401.622503]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.622902]  ? syscall_exit_to_user_mode+0x2b/0x40
[ 6401.623309]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.623703]  ? do_syscall_64+0x6c/0x90
[ 6401.624063]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.624457]  ? exit_to_user_mode_prepare+0x142/0x1f0
[ 6401.624868]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.625262]  ? syscall_exit_to_user_mode+0x2b/0x40
[ 6401.625663]  ? srso_alias_return_thunk+0x5/0x7f
[ 6401.626051]  ? do_syscall_64+0x6c/0x90
[ 6401.626404]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 6401.626810] RIP: 0033:0x7f2ffb321594
[ 6401.627164] Code: 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 80 3d d5 cd 0d 00 00 74 13 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 3c c3 0f 1f 00 55 48 89 e5 48 83 ec 10 89 7d
[ 6401.628345] RSP: 002b:00007ffe8d8eef18 EFLAGS: 00000202 ORIG_RAX: 0000000000000003
[ 6401.628867] RAX: ffffffffffffffda RBX: 00007f2ffba4e668 RCX: 00007f2ffb321594
[ 6401.629372] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
[ 6401.629879] RBP: 00007ffe8d8eef40 R08: 0000000000000000 R09: 0000000000000000
[ 6401.630384] R10: 55c926e3167eae79 R11: 0000000000000202 R12: 0000000000000003
[ 6401.630889] R13: 00007ffe8d8ef030 R14: 0000000000000000 R15: 00007f2ffba4e668
[ 6401.631394]  </TASK>
[ 6401.631691] Modules linked in: tls nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr sunrpc pktcdvd intel_rapl_msr snd_hda_codec_generic intel_rapl_common ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_hwdep snd_pcm kvm_amd iTCO_wdt snd_timer intel_pmc_bxt ccp joydev iTCO_vendor_support kvm irqbypass i2c_i801 snd i2c_smbus soundcore lpc_ich virtio_balloon loop zram crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni polyval_generic ghash_clmulni_intel sha512_ssse3 sha256_ssse3 sha1_ssse3 virtio_net virtio_gpu virtio_console virtio_blk net_failover failover virtio_dma_buf serio_raw scsi_dh_rdac scsi_dh_emc scsi_dh_alua fuse dm_multipath qemu_fw_cfg
[ 6401.636126] CR2: 0000000000000010
[ 6401.636482] ---[ end trace 0000000000000000 ]---
[ 6401.636891] RIP: 0010:hrtimer_active+0xd/0x50
[ 6401.637288] Code: 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 48 8b 57 30 <8b> 42 10 a8 01 74 09 f3 90 8b 42 10 a8 01 75 f7 80 7f 38 00 75 1d
[ 6401.638507] RSP: 0018:ffffb031009b7e10 EFLAGS: 00010286
[ 6401.638951] RAX: 000000000002db00 RBX: ffff9118f786db08 RCX: 0000000000000000
[ 6401.639478] RDX: 0000000000000000 RSI: ffff9117a0e64400 RDI: ffff9118f786db08
[ 6401.640005] RBP: ffff9118f786db80 R08: ffff9117a0ddd420 R09: ffff9117804d4f70
[ 6401.640535] R10: 0000000000000000 R11: 0000000000000000 R12: ffff9118f786db08
[ 6401.641067] R13: ffff91178fdd5e20 R14: ffff9117840978c0 R15: 0000000000000000
[ 6401.641601] FS:  00007f2ffbab1740(0000) GS:ffff9118f7840000(0000) knlGS:0000000000000000
[ 6401.642174] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6401.642653] CR2: 0000000000000010 CR3: 00000001b402e000 CR4: 0000000000750ee0
[ 6401.643193] PKRU: 55555554
[ 6401.643548] note: python3[2673] exited with irqs disabled

-- Daniel

> thanks,
> 
> greg k-h
Greg KH Feb. 1, 2024, 6:05 p.m. UTC | #4 
On Thu, Feb 01, 2024 at 05:02:56PM +0100, Daniel Bristot de Oliveira wrote:
> On 2/1/24 16:44, Greg KH wrote:
> > On Thu, Feb 01, 2024 at 04:13:39PM +0100, Daniel Bristot de Oliveira wrote:
> >> Currently, the timerlat's hrtimer is initialized at the first read of
> >> timerlat_fd, and destroyed at close(). It works, but it causes an error
> >> if the user program open() and close() the file without reading.
> > 
> > What error exactly happens?  Userspace, or the kernel crashes?
> 
> sorry, kernel crash:
> 
> # echo NO_OSNOISE_WORKLOAD > /sys/kernel/debug/tracing/osnoise/options
> # echo timerlat > /sys/kernel/debug/tracing/current_tracer
> 
> # cat ./timerlat_load.py
> #!/usr/bin/env python3
> 
> timerlat_fd = open("/sys/kernel/tracing/osnoise/per_cpu/cpu0/timerlat_fd", 'r')
> timerlat_fd.close();
> 
> # ./taskset -c 0 ./timerlat_load.py
> <BOOM>

Then obviously, this is a real, functional, change, so say so in the
kernel changelog :)

thanks,

greg k-h
diff mbox series

Patch

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index bd0d01d00fb9..a8e28f9b9271 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2444,6 +2444,9 @@  static int timerlat_fd_open(struct inode *inode, struct file *file)
 	tlat = this_cpu_tmr_var();
 	tlat->count = 0;
 
+	hrtimer_init(&tlat->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
+	tlat->timer.function = timerlat_irq;
+
 	migrate_enable();
 	return 0;
 };
@@ -2526,9 +2529,6 @@  timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 		tlat->tracing_thread = false;
 		tlat->kthread = current;
 
-		hrtimer_init(&tlat->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD);
-		tlat->timer.function = timerlat_irq;
-
 		/* Annotate now to drift new period */
 		tlat->abs_period = hrtimer_cb_get_time(&tlat->timer);