[apparmor] [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.

[NeilBrown]

On Tue, 24 Feb 2026, Chris Mason wrote:
> NeilBrown <neilb at ownmail.net> wrote:
> > From: NeilBrown <neil at brown.name>
> > 
> > When ovl_create_real() is used to create a file on the upper filesystem
> > it needs to return the resulting dentry - positive and hashed.
> > It is usually the case the that dentry passed to the create function
> > (e.g.  vfs_create()) will be suitable but this is not guaranteed.  The
> > filesystem may unhash that dentry forcing a repeat lookup next time the
> > name is wanted.
> > 
> 
> Hi everyone,
> 
> Amir suggested I run these through, and this commit was flagged:
> 
> commit 62d49d1e44667e4f93bec415faabec5526992ac0
> Author: NeilBrown <neil at brown.name>
> 
> ovl: change ovl_create_real() to get a new lock when re-opening created file.
> 
> This commit changes ovl_create_real() to drop the directory lock and
> reacquire a new lock for lookup when the created dentry is unhashed. It
> also removes ovl_lookup_upper() which is no longer used.
> 
> Signed-off-by: NeilBrown <neil at brown.name>
> 
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index c4feb89ad1e39..6285069ccc590 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> >  }
> >
> >  struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> > -			       struct dentry *newdentry, struct ovl_cattr *attr)
> > +			       struct dentry *newdentry, struct qstr *qname,
> > +			       struct ovl_cattr *attr)
> >  {
> >  	struct inode *dir = parent->d_inode;
> >  	int err;
> > @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> >  		struct dentry *d;
> >  		/*
> >  		 * Some filesystems (i.e. casefolded) may return an unhashed
> > -		 * negative dentry from the ovl_lookup_upper() call before
> > +		 * negative dentry from the ovl_start_creating_upper() call before
> >  		 * ovl_create_real().
> >  		 * In that case, lookup again after making the newdentry
> >  		 * positive, so ovl_create_upper() always returns a hashed
> > -		 * positive dentry.
> > +		 * positive dentry.  We lookup using qname which should be
> > +		 * the same name as newentry, but is certain not to change.
> > +		 * As we have to drop the lock before the lookup a race
> > +		 * could result in a lookup failure.  In that case we return
> > +		 * an error.
> >  		 */
> > -		d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> > -				     newdentry->d_name.len);
> > -		dput(newdentry);
> > -		if (IS_ERR_OR_NULL(d))
> > +		end_creating_keep(newdentry);
> > +		d = ovl_start_creating_upper(ofs, parent, qname);
> > +
> > +		if (IS_ERR_OR_NULL(d)) {
> >  			err = d ? PTR_ERR(d) : -ENOENT;
> > +		} else if (d->d_inode != newdentry->d_inode) {
> > +			err = -EIO;
> > +			dput(newdentry);
> > +		} else {
> > +			dput(newdentry);
> >  			return d;
> > +		}
> > +		return ERR_PTR(err);
> >  	}
> 
> Does this leak the newdentry reference when IS_ERR_OR_NULL(d) is true?
> 
> The code now calls end_creating_keep(newdentry) which keeps the reference,
> then calls ovl_start_creating_upper(). If that returns an error or NULL,
> the function returns ERR_PTR(err) without cleaning up newdentry:
> 
> 	end_creating_keep(newdentry);
> 	d = ovl_start_creating_upper(ofs, parent, qname);
> 
> 	if (IS_ERR_OR_NULL(d)) {
> 		err = d ? PTR_ERR(d) : -ENOENT;
> 	} ...
> 	return ERR_PTR(err);
> 
> The old code always went through the 'out:' label which called
> end_creating(newdentry) on error. The new code bypasses that cleanup.
> 
> 
> Does this leak both the parent lock and the dentry 'd' when the lookup
> returns a different inode?
> 
> When d->d_inode != newdentry->d_inode, the code sets err = -EIO and calls
> dput(newdentry), but doesn't clean up 'd':
> 
> 	} else if (d->d_inode != newdentry->d_inode) {
> 		err = -EIO;
> 		dput(newdentry);
> 	} ...
> 	return ERR_PTR(err);
> 
> The dentry 'd' was returned by ovl_start_creating_upper() with the parent
> directory lock held. The code needs to call end_creating(d) to release the
> lock via end_dirop()->inode_unlock() and release the dentry reference via
> end_dirop()->dput().
> 
> 

Yes, that code is rather messed up - thanks.

I've made it:

		end_creating_keep(newdentry);
		d = ovl_start_creating_upper(ofs, parent, qname);

		if (IS_ERR_OR_NULL(d)) {
			err = d ? PTR_ERR(d) : -ENOENT;
		} else if (d->d_inode != newdentry->d_inode) {
			err = -EIO;
		} else {
			dput(newdentry);
			return d;
		}
		end_creating(d);
		dput(newdentry);
		return ERR_PTR(err);

Thanks,
NeilBrown