Re: [PATCH 11/11] ecryptfs: use new start_creaing/start_removing APIs

[NeilBrown <neilb@ownmail.net>]

On Sun, 28 Sep 2025, Amir Goldstein wrote:
> On Fri, Sep 26, 2025 at 4:51 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > This requires the addition of start_creating_dentry().
> >
...
> > @@ -186,10 +190,12 @@ ecryptfs_do_create(struct inode *directory_inode,
> >         struct inode *lower_dir;
> >         struct inode *inode;
> >
> > -       rc = lock_parent(ecryptfs_dentry, &lower_dentry, &lower_dir);
> > -       if (!rc)
> > -               rc = vfs_create(&nop_mnt_idmap, lower_dir,
> > -                               lower_dentry, mode, true);
> > +       lower_dentry = ecryptfs_start_creating_dentry(ecryptfs_dentry);
> > +       if (IS_ERR(lower_dentry))
> > +               return ERR_CAST(lower_dentry);
> > +       lower_dir = lower_dentry->d_parent->d_inode;
> > +       rc = vfs_create(&nop_mnt_idmap, lower_dir,
> > +                       lower_dentry, mode, true);
> >         if (rc) {
> >                 printk(KERN_ERR "%s: Failure to create dentry in lower fs; "
> >                        "rc = [%d]\n", __func__, rc);
> > @@ -205,7 +211,7 @@ ecryptfs_do_create(struct inode *directory_inode,
> >         fsstack_copy_attr_times(directory_inode, lower_dir);
> >         fsstack_copy_inode_size(directory_inode, lower_dir);
> >  out_lock:
> > -       inode_unlock(lower_dir);
> > +       end_creating(lower_dentry, NULL);
> 
> These calls were surprising to me.
> I did not recall any documentation that @parent could be NULL
> when calling end_creating(). In fact, the documentation specifically
> says that it should be the parent used for start_creating().

I've updated the documentation for end_creating() say that the parent is
not needed when vfs_mkdir() wasn't used.

> 
> So either introduce end_creating_dentry(), which makes it clear
> that it does not take an ERR_PTR child,

it would be end_creating_not_mkdir() :-)

> Or add WARN_ON to end_creating() in case it is called with NULL
> parent and an ERR_PTR child to avoid dereferencing parent->d_inode
> in that case.

I don't think a WARN_ON is particularly useful immediately before a
NULL-pointer dereference.

Thanks for highlighting this - clarification of the documentation is
needed.

NeilBrown