On Thu 30-07-20 14:55:11, Amir Goldstein wrote: > 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) Yeah, I've noticed a similar report from Coverity. > > 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. And after looking at it my conclusion was the same. I like the symmetry of the code despite some checks are actually unnecessary... > 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. Agreed. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
