On 2023/04/06 16:54, lijiang wrote:
> On Thu, Apr 6, 2023 at 11:03 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
>
>> 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.
Yes, I see your intent. but I thought that the check itself would be
not so hard, for example, something like this.
ulong spec_addr;
spec_addr = htol(spec_string, RETURN_ON_ERROR|QUIET, NULL);
if ((spec_addr == BADADDR || !IS_KVADDR(spec_addr)) &&
spec_string[0] != '/')
error(FATAL, "invalid argument: %s\n", args[optind]);
>
>
>
>> 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.
um, this is a detail, not the point I meant, but I agree that multiple
args may be not so useful. so we may change it to not accept multiple
args, but if we do so, at least it needs a discussion separately and a
description about it in the commit log.
>
>
>> 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
> ...
Isn't this prevented by the check above?
>
>
>> (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:
sorry, (1) was a wrong condition. I meant the above check in this
email. The fuser command uses strstr() to search targets, so it may
need some trick to match paths exactly, though.
> 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
> ...
Some of these also can be prevented by the IS_KVADDR check?
>
> 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.
sorry, I lacked words..
The current "fuser" command just uses strstr() to search for targets.
It's true that it's kind of a rough search and some unexpected values
can be detected. The vm_flags is an example. The "fuser" is to search
for inodes originally and so far there is no requirement to search for
vm_flags. So it's strange to introduce that option, just because the
current fuser command detects vm_flags.
> But anyway, adding documents to prevent unusual usage is the easiest
> solution. Any thoughts?
It might be good to add a description about the fuser's behavior, too.
This is a rough one though:
The "fuser" command almost just does strstr() for "files" and "vm"
output and its output can contain some errors or noises, and kind of a
rough search. So it's better for users to check each task's detail
output for sure with the "files" and "vm" commands.
Thanks,
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
