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,
that requires an option, doesn't accept multiple args and has the -f
option that does not look useful to me. Who wants the vm_flags search?
(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
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
(2) add a supplementary description that it can be used for dentry or filp.
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.
Thanks,
Kazu
>
> Reported-by: Buland Kumar Singh <bsingh@xxxxxxxxxx>
> Signed-off-by: Lianbo Jiang <lijiang@xxxxxxxxxx>
> ---
> defs.h | 1 +
> filesys.c | 49 ++++++++++++++++++++++++++++++-------------------
> help.c | 22 ++++++++++++----------
> memory.c | 15 +++++++++++++++
> 4 files changed, 58 insertions(+), 29 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 12ad6aaa0998..d2ba00f1a4b1 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -5736,6 +5736,7 @@ char *fill_dentry_cache(ulong);
> void clear_dentry_cache(void);
> char *fill_inode_cache(ulong);
> void clear_inode_cache(void);
> +int check_vm_flags(ulonglong);
> int monitor_memory(long *, long *, long *, long *);
> int is_readable(char *);
> struct list_pair {
> diff --git a/filesys.c b/filesys.c
> index d64b54a9b822..ccc8092c10b5 100644
> --- a/filesys.c
> +++ b/filesys.c
> @@ -3378,19 +3378,19 @@ clear_inode_cache(void)
>
>
> /*
> - * This command displays the tasks using specified files or sockets.
> - * Tasks will be listed that reference the file as the current working
> - * directory, root directory, an open file descriptor, or that mmap the
> - * file.
> - * The argument can be a full pathname without symbolic links, or inode
> - * address.
> + * This command displays the tasks associated the specified files, virtual
> + * address or vm_flags. Tasks will be listed that reference the file as the
> + * current working directory, root directory, an open file descriptor, or
> + * that mmap the file.
> + * The argument can be a full pathname without symbolic links, or inode
> + * address, dentry address etc.
> */
>
> void
> cmd_fuser(void)
> {
> - int c;
> - char *spec_string, *tmp;
> + int c, pflag = 0, fflag = 0, nflag = 0;
> + char *spec_string = NULL, *tmp;
> struct foreach_data foreach_data, *fd;
> char task_buf[BUFSIZE];
> char buf[BUFSIZE];
> @@ -3399,29 +3399,42 @@ cmd_fuser(void)
> int doing_fds, doing_mmap, len;
> int fuser_header_printed, lockd_header_printed;
>
> - while ((c = getopt(argcnt, args, "")) != EOF) {
> + while ((c = getopt(argcnt, args, "p:f:n:")) != EOF) {
> switch(c)
> {
> + case 'n':
> + nflag = 1;
> + spec_string = optarg;
> + if (!file_exists(optarg, NULL) && !comm_exists(optarg))
> + error(FATAL, "invalid file name: %s\n", optarg);
> + break;
> + case 'p':
> + pflag = 1;
> + ulonglong vaddr = htoll(optarg, FAULT_ON_ERROR, NULL);
> + if (!IS_KVADDR(vaddr))
> + error(FATAL, "invalid virtual address: %s\n", optarg);
> + spec_string = optarg;
> + break;
> + case 'f':
> + fflag = 1;
> + ulonglong llvalue = htoll(optarg, FAULT_ON_ERROR, NULL);
> + if (!check_vm_flags(llvalue))
> + error(FATAL, "invalid vm flags: %s\n", optarg);
> + spec_string = optarg;
> + break;
> default:
> argerrs++;
> break;
> }
> }
>
> - if (argerrs)
> - cmd_usage(pc->curcmd, SYNOPSIS);
> -
> - if (!args[optind]) {
> + if (argerrs || (!pflag && !fflag && !nflag))
> cmd_usage(pc->curcmd, SYNOPSIS);
> - return;
> - }
>
> sprintf(fuser_header, " PID %s COMM USAGE\n",
> mkstring(buf, VADDR_PRLEN, CENTER, "TASK"));
>
> doing_fds = doing_mmap = 0;
> - while (args[optind]) {
> - spec_string = args[optind];
> if (STRNEQ(spec_string, "0x") && hexadecimal(spec_string, 0))
> shift_string_left(spec_string, 2);
> len = strlen(spec_string);
> @@ -3505,11 +3518,9 @@ cmd_fuser(void)
> BZERO(uses, 20);
> }
> close_tmpfile();
> - optind++;
> if (!fuser_header_printed && !lockd_header_printed) {
> fprintf(fp, "No users of %s found\n", spec_string);
> }
> - }
> }
>
> static void
> diff --git a/help.c b/help.c
> index 9a5cd3615589..62fca314a58e 100644
> --- a/help.c
> +++ b/help.c
> @@ -7981,18 +7981,20 @@ NULL
> char *help_fuser[] = {
> "fuser",
> "file users",
> -"[pathname | inode]",
> -" This command displays the tasks using specified files or sockets.",
> -" Tasks will be listed that reference the file as the current working",
> -" directory, root directory, an open file descriptor, or that mmap the",
> -" file. If the file is held open in the kernel by the lockd server on",
> -" behalf of a client discretionary file lock, the client hostname is",
> -" listed.\n",
> -" pathname the full pathname of the file.",
> -" inode the hexadecimal inode address for the file.",
> +"[-p addr | -n filename | -f vm_flags]",
> +" This command displays the tasks associated the specified files, virtual",
> +" address or vm_flags. Tasks will be listed that reference the file as the",
> +" current working, directory, root directory, an open file descriptor, or",
> +" that mmap the file. If the file is held open in the kernel by the lockd",
> +" server on behalf of a client discretionary file lock, the client hostname",
> +" is listed.\n",
> +" -n filename the full pathname of the file.",
> +" -p addr the hexadecimal virtual address for the file. It may be a",
> +" valid inode address, dentry address, file address, etc.",
> +" -f vm_flags the FLAGS (vm_flags) value."
> "\nEXAMPLES",
> " Display the tasks using file /usr/lib/libkfm.so.2.0.0\n",
> -" %s> fuser /usr/lib/libkfm.so.2.0.0",
> +" %s> fuser -n /usr/lib/libkfm.so.2.0.0",
> " PID TASK COMM USAGE",
> " 779 c5e82000 \"kwm\" mmap",
> " 808 c5a8e000 \"krootwm\" mmap",
> diff --git a/memory.c b/memory.c
> index 0568f18eb9b7..b2c12f146d95 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -3727,6 +3727,21 @@ cmd_vm(void)
> #define VM_PFN_AT_MMAP 0x40000000ULL /* PFNMAP vma that is fully mapped at mmap time */
> #define VM_MERGEABLE 0x80000000ULL /* KSM may merge identical pages */
>
> +int check_vm_flags(ulonglong flags)
> +{
> + if ( flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED|
> + VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC|VM_MAYSHARE|
> + VM_GROWSDOWN|VM_GROWSUP|VM_SHM|VM_DENYWRITE|
> + VM_EXECUTABLE|VM_LOCKED|VM_IO|VM_SEQ_READ|
> + VM_RAND_READ|VM_DONTCOPY|VM_DONTEXPAND|VM_RESERVED|
> + VM_BIGPAGE|VM_BIGMAP|VM_HUGETLB|VM_NONLINEAR|
> + VM_MAPPED_COPY|VM_INSERTPAGE|VM_ALWAYSDUMP|VM_CAN_NONLINEAR|
> + VM_MIXEDMAP|VM_SAO|VM_PFN_AT_MMAP|VM_MERGEABLE))
> + return TRUE;
> +
> + return FALSE;
> +}
> +
> static void
> do_vm_flags(ulonglong flags)
> {
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
