On 2023/04/06 16:54, lijiang wrote: 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]); 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. Isn't this prevented by the check above? 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. Some of these also can be prevented by the IS_KVADDR check? 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. 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

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