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

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.