On Wed, Feb 8, 2017 at 9:42 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Tue, Jan 31, 2017 at 9:15 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>> 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.
>>
>
> Ping.
>
> Would you mind queuing this one as well for 4.11?
>
Ping^2
This fixes a false positive lockdep splat on new xfstest overlay/029.
> Thanks,
> Amir.
>
>>
>>> 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
