On Wed, Jan 11, 2017 at 12:07 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> An overlayfs instance can be the lower layer of another overlayfs
> instance. This setup triggers a lockdep splat of possible recursive
> locking of sb->s_type->i_mutex_key in iterate_dir(). Trimmed snip:
>
> [ INFO: possible recursive locking detected ]
> bash/2468 is trying to acquire lock:
> &sb->s_type->i_mutex_key#14, at: iterate_dir+0x7d/0x15c
> but task is already holding lock:
> &sb->s_type->i_mutex_key#14, at: iterate_dir+0x7d/0x15c
>
> One problem observed with this splat is that ovl_new_inode()
> does not call lockdep_annotate_inode_mutex_key() to annotate
> the dir inode lock as &sb->s_type->i_mutex_dir_key like other
> fs do.
>
> The other problem is that the 2 nested levels of overlayfs inode
> lock are annotated using the same key, which is the cause of the
> false positive lockdep warning.
>
> Fix this by annotating overlayfs inode lock in ovl_fill_inode()
> according to stack level of the super block instance and use
> different key for dir vs. non-dir like other fs do.
>
> Here is an edited snip from /proc/lockdep_chains after
> iterate_dir() of nested overlayfs:
>
> [...] &ovl_i_mutex_dir_key[depth] (stack_depth=2)
> [...] &ovl_i_mutex_dir_key[depth]#2 (stack_depth=1)
> [...] &type->i_mutex_dir_key (stack_depth=0)
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> fs/overlayfs/inode.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> v4:
> - further simplify by removing unneeded ofs->nested
>
Miklos,
This patch version addresses your review comments from previous merge cycle
and it has zero impact without CONFIG_LOCKDEP as you requested.
> v3:
> - discard different annotation for nesting level 0
> - compile away without CONFIG_LOCKDEP
>
> v2:
> - specific implementation in overlayfs
>
> v1:
> - generic implemetnation in vfs
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 08643ac..b954622 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -301,6 +301,42 @@ static const struct inode_operations ovl_symlink_inode_operations = {
> .update_time = ovl_update_time,
> };
>
> +/*
> + * It is possible to stack overlayfs instance on top of another
> + * overlayfs instance as lower layer. We need to annonate the
> + * stackable i_mutex locks according to stack level of the super
> + * block instance. An overlayfs instance can never be in stack
> + * depth 0 (there is always a real fs below it). An overlayfs
> + * inode lock will use the lockdep annotaion ovl_i_mutex_key[depth].
> + *
> + * For example, here is a snip from /proc/lockdep_chains after
> + * dir_iterate of nested overlayfs:
> + *
> + * [...] &ovl_i_mutex_dir_key[depth] (stack_depth=2)
> + * [...] &ovl_i_mutex_dir_key[depth]#2 (stack_depth=1)
> + * [...] &type->i_mutex_dir_key (stack_depth=0)
> + */
> +
> +static struct lock_class_key ovl_i_mutex_key[FILESYSTEM_MAX_STACK_DEPTH];
> +static struct lock_class_key ovl_i_mutex_dir_key[FILESYSTEM_MAX_STACK_DEPTH];
> +
> +static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
> +{
> +#ifdef CONFIG_LOCKDEP
> + int depth = inode->i_sb->s_stack_depth - 1;
> +
> + if (WARN_ON_ONCE(depth < 0))
> + depth = 0;
> +
> + if (S_ISDIR(inode->i_mode))
> + lockdep_set_class(&inode->i_rwsem,
> + &ovl_i_mutex_dir_key[depth]);
> + else
> + lockdep_set_class(&inode->i_rwsem,
> + &ovl_i_mutex_key[depth]);
> +#endif
> +}
> +
> static void ovl_fill_inode(struct inode *inode, umode_t mode, dev_t rdev)
> {
> inode->i_ino = get_next_ino();
> @@ -310,6 +346,8 @@ static void ovl_fill_inode(struct inode *inode, umode_t mode, dev_t rdev)
> inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE;
> #endif
>
> + ovl_lockdep_annotate_inode_mutex_key(inode);
> +
> switch (mode & S_IFMT) {
> case S_IFREG:
> inode->i_op = &ovl_file_inode_operations;
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
