Re: [PATCH] configfs: fix use-after-free in configfs_symlink() racing with rmdir()

[Drif Abdelmalek Mohamed Said]

Hi Ran,

Thanks for reviewing the patch.

There is an earlier unhash inside configfs_detach_group(child):
it calls configfs_detach_item(child), which calls
configfs_remove_dir(child). On successful simple_rmdir(),
configfs_remove_dir() calls d_drop(child) while the child's inode
lock is still held, before the later d_delete(child).

Once configfs_rmdir() proceeds to detachment, both branches reach
configfs_remove_dir():

  CONFIGFS_USET_DIR:
    configfs_detach_group(dentry)
      configfs_detach_item(dentry)
        configfs_remove_dir(dentry)

  Otherwise:
    configfs_detach_item(dentry)
      configfs_remove_dir(dentry)

For default children, detach_groups() acquires each child's inode
lock before following that same call chain.

However, d_drop() is conditional: if simple_rmdir() fails,
configfs_remove_dir() skips it. In that case, the hash check alone
does not cover the window you described. detach_groups() still sets
S_DEAD before releasing the child's inode lock, so I plan to add
an IS_DEADDIR(dir) check after relocking and before reacquiring
parent_item. That covers this alternative path without accessing
the potentially freed item.

The reference handling in my patch remains necessary: put
parent_item and set it to NULL before inode_unlock(), then obtain
it again after relocking and validating the directory. The new
failure path will release the target references via out_put_target.

I also noticed that your patch's new goto out_put still calls
config_item_put(parent_item), although its containing object may
already have been freed. It also bypasses the puts for target_sd
and target_item after a successful get_target(). Additionally, the
existing get_target() failure path still puts the stale parent.

I will include the dead-directory check in v2 and validate the
updated patch.

Thanks,
Drif