Convert hook_sb_delete() to sb_for_each_inodes() and release each callback's inode reference outside s_inode_list_lock. This removes the prev_inode reference used to preserve the walk position while retaining the synchronization with release_inode() and the final wait for pending inode releases. Signed-off-by: Julian Sun --- security/landlock/fs.c | 150 +++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 90 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 30aa6ce13590..3dcde8cbfb6a 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -1369,110 +1369,80 @@ static void hook_inode_free_security_rcu(void *inode_security) /* Super-block hooks */ -/* - * Release the inodes used in a security policy. - * - * Cf. fsnotify_unmount_inodes() and evict_inodes() - */ -static void hook_sb_delete(struct super_block *const sb) +static int hook_sb_delete_inode_iter_cb(struct inode *inode, void *data) { - struct inode *inode, *prev_inode = NULL; + struct landlock_object *object; + struct super_block *sb = inode->i_sb; - if (!landlock_initialized) - return; + if (!atomic_read(&inode->i_count)) { + spin_unlock(&inode->i_lock); + return 0; + } - spin_lock(&sb->s_inode_list_lock); - list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { - struct landlock_object *object; + rcu_read_lock(); + object = rcu_dereference(landlock_inode(inode)->object); + if (!object) { + rcu_read_unlock(); + spin_unlock(&inode->i_lock); + return 0; + } + /* Keeps a reference to this inode until the next loop walk. */ + __iget(inode); + spin_unlock(&inode->i_lock); - /* Only handles referenced inodes. */ - if (!icount_read_once(inode)) - continue; + /* + * If there is no concurrent release_inode() ongoing, then we + * are in charge of calling iput() on this inode, otherwise we + * will just wait for it to finish. + */ + spin_lock(&object->lock); + if (object->underobj == inode) { + object->underobj = NULL; + spin_unlock(&object->lock); + rcu_read_unlock(); /* - * Protects against concurrent modification of inode (e.g. - * from get_inode_object()). + * Because object->underobj was not NULL, + * release_inode() and get_inode_object() guarantee + * that it is safe to reset + * landlock_inode(inode)->object while it is not NULL. + * It is therefore not necessary to lock inode->i_lock. */ - spin_lock(&inode->i_lock); + rcu_assign_pointer(landlock_inode(inode)->object, NULL); /* - * Checks I_FREEING and I_WILL_FREE to protect against a race - * condition when release_inode() just called iput(), which - * could lead to a NULL dereference of inode->security or a - * second call to iput() for the same Landlock object. Also - * checks I_NEW because such inode cannot be tied to an object. + * At this point, we own the ihold() reference that was + * originally set up by get_inode_object() and the + * __iget() reference that we just set in this loop + * walk. Therefore there are at least two references + * on the inode. */ - if (inode_state_read(inode) & - (I_FREEING | I_WILL_FREE | I_NEW)) { - spin_unlock(&inode->i_lock); - continue; - } + iput_not_last(inode); + } else { + spin_unlock(&object->lock); + rcu_read_unlock(); + } - rcu_read_lock(); - object = rcu_dereference(landlock_inode(inode)->object); - if (!object) { - rcu_read_unlock(); - spin_unlock(&inode->i_lock); - continue; - } - /* Keeps a reference to this inode until the next loop walk. */ - __iget(inode); - spin_unlock(&inode->i_lock); + spin_unlock(&sb->s_inode_list_lock); + iput(inode); + spin_lock(&sb->s_inode_list_lock); - /* - * If there is no concurrent release_inode() ongoing, then we - * are in charge of calling iput() on this inode, otherwise we - * will just wait for it to finish. - */ - spin_lock(&object->lock); - if (object->underobj == inode) { - object->underobj = NULL; - spin_unlock(&object->lock); - rcu_read_unlock(); + return 0; +} - /* - * Because object->underobj was not NULL, - * release_inode() and get_inode_object() guarantee - * that it is safe to reset - * landlock_inode(inode)->object while it is not NULL. - * It is therefore not necessary to lock inode->i_lock. - */ - rcu_assign_pointer(landlock_inode(inode)->object, NULL); - /* - * At this point, we own the ihold() reference that was - * originally set up by get_inode_object() and the - * __iget() reference that we just set in this loop - * walk. Therefore there are at least two references - * on the inode. - */ - iput_not_last(inode); - } else { - spin_unlock(&object->lock); - rcu_read_unlock(); - } +/* + * Release the inodes used in a security policy. + * + * Cf. fsnotify_unmount_inodes() and evict_inodes() + */ +static void hook_sb_delete(struct super_block *const sb) +{ + unsigned int flags = INODE_ITER_NORMAL; - if (prev_inode) { - /* - * At this point, we still own the __iget() reference - * that we just set in this loop walk. Therefore we - * can drop the list lock and know that the inode won't - * disappear from under us until the next loop walk. - */ - spin_unlock(&sb->s_inode_list_lock); - /* - * We can now actually put the inode reference from the - * previous loop walk, which is not needed anymore. - */ - iput(prev_inode); - cond_resched(); - spin_lock(&sb->s_inode_list_lock); - } - prev_inode = inode; - } - spin_unlock(&sb->s_inode_list_lock); + if (!landlock_initialized) + return; + + sb_for_each_inodes(sb, flags, hook_sb_delete_inode_iter_cb, NULL); - /* Puts the inode reference from the last loop walk, if any. */ - if (prev_inode) - iput(prev_inode); /* Waits for pending iput() in release_inode(). */ wait_var_event(&landlock_superblock(sb)->inode_refs, !atomic_long_read(&landlock_superblock(sb)->inode_refs)); -- 2.39.5