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

On 2023/04/04 18:37, Lianbo Jiang wrote: 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(a)redhat.com&gt; > Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com&gt; > --- > 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) > {