Re: [PATCH 0/9] gfs2: Bugs in "Use GL_NOBLOCK flag for non-blocking lookups"

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Feb 02, 2024 at 04:23:13AM +0000, Al Viro wrote:
> On Fri, Jan 19, 2024 at 10:20:47PM +0100, Andreas Gruenbacher wrote:
> > Hello,
> > 
> > Al Viro has reported issues with commit dd00aaeb3432 ("gfs2: Use
> > GL_NOBLOCK flag for non-blocking lookups"):
> > 
> > * First, parent can now be NULL and dereferencing it in
> >   gfs2_dir_check(d_inode(parent), &dentry->d_name, ip) isn't going to
> >   work;
> > 
> > * Second, gfs2_dir_check() can still sleep, which breaks LOOKUP_RCU
> >   mode.
> 
> Looks sane, but there's another piece of fun in gfs_permission():
>         gl = rcu_dereference_check(ip->i_gl, !may_not_block);
>         if (unlikely(!gl)) {
>                 /* inode is getting torn down, must be RCU mode */
>                 WARN_ON_ONCE(!may_not_block);
>                 return -ECHILD;
>         }
>         if (gfs2_glock_is_locked_by_me(ip->i_gl) == NULL) {
>                 int noblock = may_not_block ? GL_NOBLOCK : 0;
>                 error = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED,
>                                            LM_FLAG_ANY | noblock, &i_gh);
>                 if (error)
>                         return error;
>         }
> 
> See the problem?  In RCU mode we carefully fetch ->i_gl and verify
> it's not NULL.  Then we proceed to dereference it again.
> 
> IOW, these ip->i_gl below ought to be replaced with gl, or you are
> risking a compiler fetching the sucker again and getting NULL this
> time around.

fix breakage in gfs2_permission()

"gfs2: Use GL_NOBLOCK flag for non-blocking lookups" has
reintroduced the bug fixed in "gfs2: fix an oops in
gfs2_permission" a while ago.  We still fetch ->i_gl carefully,
and check if it's NULL, but then we proceed to fetch it again.
If it became NULL in the meanwhile, we get trouble...

Trivial to fix, fortunately.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 6bfc9383b7b8..d045562bf08f 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1882,9 +1882,9 @@ int gfs2_permission(struct mnt_idmap *idmap, struct inode *inode,
 		WARN_ON_ONCE(!may_not_block);
 		return -ECHILD;
         }
-	if (gfs2_glock_is_locked_by_me(ip->i_gl) == NULL) {
+	if (gfs2_glock_is_locked_by_me(gl) == NULL) {
 		int noblock = may_not_block ? GL_NOBLOCK : 0;
-		error = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED,
+		error = gfs2_glock_nq_init(gl, LM_ST_SHARED,
 					   LM_FLAG_ANY | noblock, &i_gh);
 		if (error)
 			return error;