* [bug report] fsnotify: pass dir and inode arguments to fsnotify() @ 2020-07-30 11:13 dan.carpenter 2020-07-30 11:55 ` Amir Goldstein 0 siblings, 1 reply; 3+ messages in thread From: dan.carpenter @ 2020-07-30 11:13 UTC (permalink / raw) To: amir73il; +Cc: linux-fsdevel 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: 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. 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? 461 (!child || !child->i_fsnotify_marks)) 462 return 0; regards, dan carpenter ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] fsnotify: pass dir and inode arguments to fsnotify() 2020-07-30 11:13 [bug report] fsnotify: pass dir and inode arguments to fsnotify() dan.carpenter @ 2020-07-30 11:55 ` Amir Goldstein 2020-07-30 19:25 ` Jan Kara 0 siblings, 1 reply; 3+ messages in thread From: Amir Goldstein @ 2020-07-30 11:55 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-fsdevel, Jan Kara On Thu, Jul 30, 2020 at 2:13 PM <dan.carpenter@oracle.com> 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] fsnotify: pass dir and inode arguments to fsnotify() 2020-07-30 11:55 ` Amir Goldstein @ 2020-07-30 19:25 ` Jan Kara 0 siblings, 0 replies; 3+ messages in thread From: Jan Kara @ 2020-07-30 19:25 UTC (permalink / raw) To: Amir Goldstein; +Cc: Dan Carpenter, linux-fsdevel, Jan Kara On Thu 30-07-20 14:55:11, Amir Goldstein wrote: > On Thu, Jul 30, 2020 at 2:13 PM <dan.carpenter@oracle.com> 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@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-30 19:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-30 11:13 [bug report] fsnotify: pass dir and inode arguments to fsnotify() dan.carpenter 2020-07-30 11:55 ` Amir Goldstein 2020-07-30 19:25 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).