| Message ID | 20220325190001.1832-1-dossche.niels@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/dcache: use lockdep assertion instead of warn try_lock | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, 2022-03-25 at 20:00 +0100, Niels Dossche wrote: > Currently, there is a fallback with a WARN that uses down_read_trylock > as a safety measure for when there is no lock taken. The current > callsites expect a write lock to be taken. Moreover, the s_root field > is written to, which is not allowed under a read lock. > This code safety fallback should not be executed unless there is an > issue somewhere else. > Using a lockdep assertion better communicates the intent of the code, > and gets rid of the currently slightly wrong fallback solution. > > Note: > I am currently working on a static analyser to detect missing locks > using type-based static analysis as my master's thesis > in order to obtain my master's degree. > If you would like to have more details, please let me know. > This was a reported case. I manually verified the report by looking > at the code, so that I do not send wrong information or patches. > After concluding that this seems to be a true positive, I created > this patch. I have both compile-tested this patch and runtime-tested > this patch on x86_64. The effect on a running system could be a > potential race condition in exceptional cases. > This issue was found on Linux v5.17. > > Fixes: c636ebdb186bf ("VFS: Destroy the dentries contributed by a superblock on unmounting") > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > --- > fs/dcache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/dcache.c b/fs/dcache.c > index c84269c6e8bf..0142f15340e5 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -1692,7 +1692,7 @@ void shrink_dcache_for_umount(struct super_block *sb) > { > struct dentry *dentry; > > - WARN(down_read_trylock(&sb->s_umount), "s_umount should've been locked"); > + lockdep_assert_held_write(&sb->s_umount); > > dentry = sb->s_root; > sb->s_root = NULL; Much nicer. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, Mar 25, 2022 at 08:00:02PM +0100, Niels Dossche wrote: > Currently, there is a fallback with a WARN that uses down_read_trylock > as a safety measure for when there is no lock taken. The current > callsites expect a write lock to be taken. Moreover, the s_root field > is written to, which is not allowed under a read lock. > This code safety fallback should not be executed unless there is an > issue somewhere else. > Using a lockdep assertion better communicates the intent of the code, > and gets rid of the currently slightly wrong fallback solution. > > Note: > I am currently working on a static analyser to detect missing locks > using type-based static analysis as my master's thesis > in order to obtain my master's degree. > If you would like to have more details, please let me know. > This was a reported case. I manually verified the report by looking > at the code, so that I do not send wrong information or patches. > After concluding that this seems to be a true positive, I created > this patch. I have both compile-tested this patch and runtime-tested > this patch on x86_64. The effect on a running system could be a > potential race condition in exceptional cases. > This issue was found on Linux v5.17. > > Fixes: c636ebdb186bf ("VFS: Destroy the dentries contributed by a superblock on unmounting") > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > --- Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
diff --git a/fs/dcache.c b/fs/dcache.c index c84269c6e8bf..0142f15340e5 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1692,7 +1692,7 @@ void shrink_dcache_for_umount(struct super_block *sb) { struct dentry *dentry; - WARN(down_read_trylock(&sb->s_umount), "s_umount should've been locked"); + lockdep_assert_held_write(&sb->s_umount); dentry = sb->s_root; sb->s_root = NULL;
Currently, there is a fallback with a WARN that uses down_read_trylock as a safety measure for when there is no lock taken. The current callsites expect a write lock to be taken. Moreover, the s_root field is written to, which is not allowed under a read lock. This code safety fallback should not be executed unless there is an issue somewhere else. Using a lockdep assertion better communicates the intent of the code, and gets rid of the currently slightly wrong fallback solution. Note: I am currently working on a static analyser to detect missing locks using type-based static analysis as my master's thesis in order to obtain my master's degree. If you would like to have more details, please let me know. This was a reported case. I manually verified the report by looking at the code, so that I do not send wrong information or patches. After concluding that this seems to be a true positive, I created this patch. I have both compile-tested this patch and runtime-tested this patch on x86_64. The effect on a running system could be a potential race condition in exceptional cases. This issue was found on Linux v5.17. Fixes: c636ebdb186bf ("VFS: Destroy the dentries contributed by a superblock on unmounting") Suggested-by: Christoph Hellwig <hch@infradead.org> Signed-off-by: Niels Dossche <dossche.niels@gmail.com> --- fs/dcache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)