On Thu, Jul 30, 2020 at 2:13 PM <dan.carpenter@xxxxxxxxxx> wrote: > > Hello Amir Goldstein, > > This is a semi-automatic email about new static checker warnings. > > The patch 40a100d3adc1: "fsnotify: pass dir and inode arguments to > fsnotify()" from Jul 22, 2020, leads to the following Smatch > complaint: That's an odd report, because... > > fs/notify/fsnotify.c:460 fsnotify() > warn: variable dereferenced before check 'inode' (see line 449) > > fs/notify/fsnotify.c > 448 } > 449 sb = inode->i_sb; > ^^^^^^^^^^^ > New dreference. First of all, two lines above we have if (!inode) inode = dir; This function does not assert (inode || dir), but must it?? This is even documented: * @inode: optional inode associated with event - * either @dir or @inode must be non-NULL. Second, The line above was indeed added by: 40a100d3adc1: "fsnotify: pass dir and inode arguments to fsnotify()" However... > > 450 > 451 /* > 452 * Optimization: srcu_read_lock() has a memory barrier which can > 453 * be expensive. It protects walking the *_fsnotify_marks lists. > 454 * However, if we do not walk the lists, we do not have to do > 455 * SRCU because we have no references to any objects and do not > 456 * need SRCU to keep them "alive". > 457 */ > 458 if (!sb->s_fsnotify_marks && > 459 (!mnt || !mnt->mnt_fsnotify_marks) && > 460 (!inode || !inode->i_fsnotify_marks) && > ^^^^^^ > Check too late. Presumably this check can be removed? But this line was only added later by: 9b93f33105f5 fsnotify: send event with parent/name info to sb/mount/non-dir marks So, yes, the check could be removed. It is a leftover from a previous revision, but even though it is a leftover I kind of like the code better this way. In principle, an event on sb/mnt that is not associated with any inode (for example FS_UNMOUNT) could be added in the future. And then we will have to fix documentation and the inode dereference above. In any case, thank you for the report, but I don't see a reason to make any changes right now. Thanks, Amir.
