Re: [Crash-utility] [PATCH] Fix "fuser" command to properly deal with an invalid argument

Hi Lianbo, Let me summarize my current suggestion: (1) make the "fuser" command do not accept an obviously invalid argument, which is not a kvaddr and also not a full path. (I think this can be done without additional options, with the check I wrote.) (2) add a note that the command does not expect an argument other than an inode address and a full path, and if others are specified, the output can be an unexpected one. What do you think? Thanks, Kazu On 2023/04/07 12:01, HAGIO KAZUHITO(萩尾 一仁) wrote: > On 2023/04/07 10:37, HAGIO KAZUHITO(萩尾 一仁) wrote: >> On 2023/04/06 16:54, lijiang wrote: >>> On Thu, Apr 6, 2023 at 11:03 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; >>> 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. > > This might be not good... > > The "vm" command in the "fuser" additionally prints the inode of files > for "USAGE" output, but does not print dentry and etc.: > > VMA START END FLAGS FILE > ffff90194db51de8 561b62898000 561b628be000 8000875 ffff900d4d239ac8 /usr/bin/less > ffff90194db50ae0 561b62abd000 561b62abe000 8100871 ffff900d4d239ac8 /usr/bin/less > ffff901877eb43a0 561b62abe000 561b62ac2000 8100873 ffff900d4d239ac8 /usr/bin/less > ^^^^^^^^^^^^^^^^ > > So the "fuser" outputs with an inode and dentry are different: > > crash-dev> files 3426630 > PID: 3426630 TASK: ffff9017ce46b080 CPU: 0 COMMAND: "less" <<-- $ less /usr/bin/less :-) > ROOT: / CWD: /root > FD FILE DENTRY INODE TYPE PATH > 0 ffff9011fabd9400 ffff901119214cc0 ffff900bbda8faf0 CHR /dev/pts/1 > 1 ffff9011fabd9400 ffff901119214cc0 ffff900bbda8faf0 CHR /dev/pts/1 > 2 ffff9011fabd9400 ffff901119214cc0 ffff900bbda8faf0 CHR /dev/pts/1 > 3 ffff90194dbeea00 ffff900b47875a40 ffff901abe07aa70 CHR /dev/tty > 4 ffff90194dbeee00 ffff901a3699a780 ffff900d4d239ac8 REG /usr/bin/less > > crash-dev> fuser ffff900d4d239ac8 <<-- inode > PID TASK COMM USAGE > 3426630 ffff9017ce46b080 "less" fd mmap <<-- there is mmap. > ... > crash-dev> fuser ffff901a3699a780 <<-- dentry > PID TASK COMM USAGE > 3426630 ffff9017ce46b080 "less" fd <<-- no mmap. > > > So the fuser does expect that arguments are inodes or full paths. > > So if we write a supplementary note, > > "This command does not expect arguments other than inodes or full paths. > If others specified, it prints a false output." > > this might be close to the real? > > Thanks, > Kazu > > >>>> >>>> >>> 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(a)redhat.com >> https://listman.redhat.com/mailman/listinfo/crash-utility >> Contribution Guidelines: https://github.com/crash-utility/crash/wiki > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://listman.redhat.com/mailman/listinfo/crash-utility > Contribution Guidelines: https://github.com/crash-utility/crash/wiki