search

ibm-s390-linux / s390-tools

Tools for use with the s390 Linux kernel and device drivers
MIT License
63 stars 59 forks source link

lscpumf fails early when run as non-root #97

Closed sharkcz closed 3 years ago

sharkcz commented 3 years ago

When lscpumf is run as non-root it prints an error message, not even accepting --help or --version options.

[sharkcz@rock-kvmlp-fedora ~]$ lscpumf -h
Error: /sys/module/kernel/parameters/cpum_sfb_size: Permission denied

I think it's because it tries to initialize itself before processing any command line options.

tmricht commented 3 years ago

The permissions for the file are -rw-r----- 1 root root 4096 Nov 11 15:08 /sys/module/kernel/parameters/cpum_sfb_size This does not allow the file to be read by ordinary users. We can change this to be word readable, however write permission will be limited to root only. Only root can use the perf tool to read counters lscpumf displays.

tmricht commented 3 years ago

The file /sys/module/kernel/parameters/cpum_sfb_size is created in the linux kernel, by the perf_cpum_cf.c device driver. Therefore there won't be any fix for that in the s390-tools. I leave it open until the fix for the linux kernel part shows up. Then you can verifiy and close it.

hoeppnerj commented 3 years ago

Actually, the issue isn't the permission of the sysfs file, but rather that information are retrieved by the tool even before the program parameters are parsed. See: https://github.com/ibm-s390-tools/s390-tools/blob/master/cpumf/lscpumf.c#L3098 (read_info() calls read_sfb() further down https://github.com/ibm-s390-tools/s390-tools/blob/master/cpumf/lscpumf.c#L2922)

I think this really should be changed so that at least --help and --version always work.

tmricht commented 3 years ago

Right, but since lscpumf simple displays some file contents of these files [root@m35lp76 linux]# ll /proc/sysinfo /proc/service_levels /sys/module/kernel/parameters/cpum_sfb_size -r--r--r-- 1 root root 0 Nov 11 16:44 /proc/service_levels -r--r--r-- 1 root root 0 Nov 11 16:43 /proc/sysinfo -rw-r----- 1 root root 4096 Nov 11 16:44 /sys/module/kernel/parameters/cpum_sfb_size [root@m35lp76 linux]#

I prefer to handle this consistently. There is no harm done with displaying sampling buffer information. Changing is a different story, that must be left to root.

hoeppnerj commented 3 years ago

Right, but since lscpumf simple displays some file contents of these files [root@m35lp76 linux]# ll /proc/sysinfo /proc/service_levels /sys/module/kernel/parameters/cpum_sfb_size -r--r--r-- 1 root root 0 Nov 11 16:44 /proc/service_levels -r--r--r-- 1 root root 0 Nov 11 16:43 /proc/sysinfo -rw-r----- 1 root root 4096 Nov 11 16:44 /sys/module/kernel/parameters/cpum_sfb_size [root@m35lp76 linux]#

I prefer to handle this consistently. There is no harm done with displaying sampling buffer information. Changing is a different story, that must be left to root.

Agreed, however, I think the behaviour of lscpumf should be changed nonetheless. Both things need to be addressed, the permissions of the sysfs attribute and the working of --help and --version should be guaranteed (as in, change the order of reading information vs parsing program parameters).

tmricht commented 3 years ago

This is correct, the --help and --version option should work in all cases. And they will work once the permissions are correct. I tried it.

hoeppnerj commented 3 years ago

This is correct, the --help and --version option should work in all cases. And they will work once the permissions are correct. I tried it.

Only for this case. The design of the tool is still broken in that regard and if in the future someone changes the code, it might break again. Please make sure the tool gets fixed as well.

tmricht commented 3 years ago

ok, will write a patch to fix this.