On 2023/04/04 18:37, Lianbo Jiang wrote:
> The man page of the "fuser" command suggests that the argument can be a
> full pathname or inode address. However, the "fuser" command accepts an
> invalid argument and prints a bogus result as below:
>
> crash> fuser x
> PID TASK COMM USAGE
> 100507 ffff9914431f4c80 "packagekitd" fd
> 100508 ffff991574e59980 "gmain" fd
> 100509 ffff9914431f3300 "gdbus" fd
> 102020 ffff991574400000 "sshd" fd
> 102043 ffff991441d19980 "sshd" fd
>
> The current fuser command has no checking mechanism to determine if an
> argument is valid or not. Lets add it to handle such cases.
>
> In addition, the fuser can also accept the dentry and file address, for
> example:
>
> crash> files -d ffff894d826e7240
> DENTRY INODE SUPERBLK TYPE PATH
> ffff894d826e7240 ffff894d805b7520 ffff894d80ce5000 REG /root/proc/kcore
> crash> fuser ffff894d826e7240
> PID TASK COMM USAGE
> 1237273 ffff89502ddf0000 "crash" fd
> 1237280 ffff895079cb0000 "gdb worker" fd
> 1237281 ffff894f9a124000 "gdb worker" fd
> 1237282 ffff895017a28000 "gdb worker" fd
> 1237283 ffff894d89c40000 "gdb worker" fd
> ...
>
> Essentially, the fuser command still calls the vm(vm_area_dump())
> and files(open_files_dump()) commands(see foreach()) to achieve
> this goal. So the man page needs to be updated, and there is no
> functional change.
>
> With the patch:
> crash> fuser -n x
> fuser: invalid file name: x
> crash> fuser -p x
> fuser: invalid virtual address: x
>
> crash> fuser -p 0xffff894d826e7240
> PID TASK COMM USAGE
> 1237273 ffff89502ddf0000 "crash" fd
> 1237280 ffff895079cb0000 "gdb worker" fd
> 1237281 ffff894f9a124000 "gdb worker" fd
> 1237282 ffff895017a28000 "gdb worker" fd
> 1237283 ffff894d89c40000 "gdb worker" fd
> ...
Sorry, but I don't understand why you chose this big interface change,
The initial intent is to make a distinction between an address and a string(file name), so that we can easily check if the argument is valid.
that requires an option, doesn't accept multiple args and has the -f
The displaying result of multiple args looks not very clear(confused). This was replaced with a single arg in the patch, but it still can get similar results by running the fuser command repeatedly, and looks cleaner.
option that does not look useful to me. Who wants the vm_flags search?
Agree with you. But I cannot prevent this situation because someone is using it like this. :-) For example:
crash> vm -f 8000075
8000075: (READ|EXEC|MAYREAD|MAYWRITE|MAYEXEC|CAN_NONLINEAR)
crash> fuser 8000075
PID TASK COMM USAGE
1 ffff894d80270000 "systemd" mmap
548 ffff894d8789c000 "systemd-journa mmap
561 ffff894d841bc000 "systemd-udevd" mmap
685 ffff894d819b8000 "systemd-oomd" mmap
686 ffff894d885a0000 "systemd-resolv mmap
687 ffff894d871cc000 "systemd-userdb mmap
688 ffff894d841b4000 "auditd" mmap
689 ffff894d85810000 "auditd" mmap
713 ffff894d84e04000 "avahi-daemon" mmap
714 ffff894d84e08000 "bluetoothd" mmap
717 ffff894d84e0c000 "mcelog" mmap
718 ffff894d84060000 "polkitd" mmap
719 ffff894d8c174000 "power-profiles mmap
720 ffff894da2084000 "rtkit-daemon" mmap
723 ffff894da2088000 "accounts-daemo mmap
724 ffff894da208c000 "switcheroo-con mmap
726 ffff894d86c40000 "systemd-logind mmap
728 ffff894d8719c000 "systemd-machin mmap
...
(Personally I think that the user that runs "fuser x" knows what they
did, so it prints some but they will just ignore the result. It does
Agree, but that is an ideal situation.
not look so bad to me in the first place.. but ok, there may be some
confusion with valid addresses.)
Can't we handle it better like this?
(1) add some check for arg
if arg is not KVADDR || arg does not starts with '/'
error out
Tried it. But actually the current fuser command still can accept a single file name(without full pathname arg) as below:
crash> fuser crash
PID TASK COMM USAGE
2760 ffff894ef2204000 "bash" cwd
5772 ffff894d80d34000 "bash" cwd
153672 ffff8952abd14000 "bash" cwd
270976 ffff89534e500000 "bash" cwd
482087 ffff894e1660c000 "bash" cwd
580499 ffff89518a160000 "vim" cwd
...
For example:
[1] fuser x
[2] fuser 0
[3] fuser 00
[4] fuser CHR
[5] fuser CWD
[6] fuser ROOT
[7] fuser root
...
(2) add a supplementary description that it can be used for dentry or filp.
Good idea, partially updated to the man page.
In addition, the fuser command can also accept the following address args:
[1] struct inode address
[2] struct dentry address
[3] struct file address
[4] struct vm_area_struct(VMA) address
[5] struct vm_area_struct.vm_start value
[6] struct vm_area_struct.vm_end value
[7] struct vm_area_struct.vm_flags value
...
I don't think that the help text must explain perfectly how it works.
Introducing a strange interface to satisfy it is giving priority to a
less important thing, I think.
That depends on how you think about it. From the perspective of users and testing, the above cases seem reasonable. And It's not easy to cover these cases without the interface changes.
But anyway, adding documents to prevent unusual usage is the easiest solution. Any thoughts?
Thanks.
Lianbo