On Mon, Oct 02, 2023 at 01:59:46PM +0100, Al Viro wrote:
> On Mon, Oct 02, 2023 at 06:46:03AM -0500, Bob Peterson wrote:
> > > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > > index 0eac04507904..e2432c327599 100644
> > > --- a/fs/gfs2/inode.c
> > > +++ b/fs/gfs2/inode.c
> > > @@ -1868,14 +1868,16 @@ int gfs2_permission(struct mnt_idmap *idmap, struct inode *inode,
> > > {
> > > struct gfs2_inode *ip;
> > > struct gfs2_holder i_gh;
> > > + struct gfs2_glock *gl;
> > > int error;
> > > gfs2_holder_mark_uninitialized(&i_gh);
> > > ip = GFS2_I(inode);
> > > - if (gfs2_glock_is_locked_by_me(ip->i_gl) == NULL) {
> > > + gl = rcu_dereference(ip->i_gl);
> > > + if (!gl || gfs2_glock_is_locked_by_me(gl) == NULL) {
> >
> > This looks wrong. It should be if (gl && ... otherwise the
> > gfs2_glock_nq_init will dereference the null pointer.
>
> We shouldn't observe NULL ->i_gl unless we are in RCU mode,
> which means we'll bail out without reaching gfs2_glock_nq_init()...
Something like
if (unlikely(!gl)) {
/* inode is getting torn down, must be RCU mode */
WARN_ON_ONCE(!(mask & MAY_NOT_BLOCK));
return -ECHILD;
}
might be less confusing way to express that...
