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

[lijiang <lijiang@xxxxxxxxxx>]

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.

 

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

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki