Commit feb3c766 authored by John Johansen's avatar John Johansen

apparmor: fix possible recursive lock warning in __aa_create_ns

Use mutex_lock_nested to provide lockdep the parent child lock ordering of
the tree.

This fixes the lockdep Warning
[  305.275177] ============================================
[  305.275178] WARNING: possible recursive locking detected
[  305.275179] 4.14.0-rc7+ #320 Not tainted
[  305.275180] --------------------------------------------
[  305.275181] apparmor_parser/1339 is trying to acquire lock:
[  305.275182]  (&ns->lock){+.+.}, at: [<ffffffff970544dd>] __aa_create_ns+0x6d/0x1e0
[  305.275187]
               but task is already holding lock:
[  305.275187]  (&ns->lock){+.+.}, at: [<ffffffff97054b5d>] aa_prepare_ns+0x3d/0xd0
[  305.275190]
               other info that might help us debug this:
[  305.275191]  Possible unsafe locking scenario:

[  305.275192]        CPU0
[  305.275193]        ----
[  305.275193]   lock(&ns->lock);
[  305.275194]   lock(&ns->lock);
[  305.275195]
                *** DEADLOCK ***

[  305.275196]  May be due to missing lock nesting notation

[  305.275198] 2 locks held by apparmor_parser/1339:
[  305.275198]  #0:  (sb_writers#10){.+.+}, at: [<ffffffff96e9c6b7>] vfs_write+0x1a7/0x1d0
[  305.275202]  #1:  (&ns->lock){+.+.}, at: [<ffffffff97054b5d>] aa_prepare_ns+0x3d/0xd0
[  305.275205]
               stack backtrace:
[  305.275207] CPU: 1 PID: 1339 Comm: apparmor_parser Not tainted 4.14.0-rc7+ #320
[  305.275208] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.1-1ubuntu1 04/01/2014
[  305.275209] Call Trace:
[  305.275212]  dump_stack+0x85/0xcb
[  305.275214]  __lock_acquire+0x141c/0x1460
[  305.275216]  ? __aa_create_ns+0x6d/0x1e0
[  305.275218]  ? ___slab_alloc+0x183/0x540
[  305.275219]  ? ___slab_alloc+0x183/0x540
[  305.275221]  lock_acquire+0xed/0x1e0
[  305.275223]  ? lock_acquire+0xed/0x1e0
[  305.275224]  ? __aa_create_ns+0x6d/0x1e0
[  305.275227]  __mutex_lock+0x89/0x920
[  305.275228]  ? __aa_create_ns+0x6d/0x1e0
[  305.275230]  ? trace_hardirqs_on_caller+0x11f/0x190
[  305.275231]  ? __aa_create_ns+0x6d/0x1e0
[  305.275233]  ? __lockdep_init_map+0x57/0x1d0
[  305.275234]  ? lockdep_init_map+0x9/0x10
[  305.275236]  ? __rwlock_init+0x32/0x60
[  305.275238]  mutex_lock_nested+0x1b/0x20
[  305.275240]  ? mutex_lock_nested+0x1b/0x20
[  305.275241]  __aa_create_ns+0x6d/0x1e0
[  305.275243]  aa_prepare_ns+0xc2/0xd0
[  305.275245]  aa_replace_profiles+0x168/0xf30
[  305.275247]  ? __might_fault+0x85/0x90
[  305.275250]  policy_update+0xb9/0x380
[  305.275252]  profile_load+0x7e/0x90
[  305.275254]  __vfs_write+0x28/0x150
[  305.275256]  ? rcu_read_lock_sched_held+0x72/0x80
[  305.275257]  ? rcu_sync_lockdep_assert+0x2f/0x60
[  305.275259]  ? __sb_start_write+0xdc/0x1c0
[  305.275261]  ? vfs_write+0x1a7/0x1d0
[  305.275262]  vfs_write+0xca/0x1d0
[  305.275264]  ? trace_hardirqs_on_caller+0x11f/0x190
[  305.275266]  SyS_write+0x49/0xa0
[  305.275268]  entry_SYSCALL_64_fastpath+0x23/0xc2
[  305.275271] RIP: 0033:0x7fa6b22e8c74
[  305.275272] RSP: 002b:00007ffeaaee6288 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[  305.275273] RAX: ffffffffffffffda RBX: 00007ffeaaee62a4 RCX: 00007fa6b22e8c74
[  305.275274] RDX: 0000000000000a51 RSI: 00005566a8198c10 RDI: 0000000000000004
[  305.275275] RBP: 0000000000000a39 R08: 0000000000000a51 R09: 0000000000000000
[  305.275276] R10: 0000000000000000 R11: 0000000000000246 R12: 00005566a8198c10
[  305.275277] R13: 0000000000000004 R14: 00005566a72ecb88 R15: 00005566a72ec3a8

Fixes: 73688d1e ("apparmor: refactor prepare_ns() and make usable from different views")
Signed-off-by: default avatarJohn Johansen <john.johansen@canonical.com>
parent 5d7c44ef
...@@ -533,7 +533,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf, ...@@ -533,7 +533,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf,
long last_read; long last_read;
int avail; int avail;
mutex_lock(&rev->ns->lock); mutex_lock_nested(&rev->ns->lock, rev->ns->level);
last_read = rev->last_read; last_read = rev->last_read;
if (last_read == rev->ns->revision) { if (last_read == rev->ns->revision) {
mutex_unlock(&rev->ns->lock); mutex_unlock(&rev->ns->lock);
...@@ -543,7 +543,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf, ...@@ -543,7 +543,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf,
last_read != last_read !=
READ_ONCE(rev->ns->revision))) READ_ONCE(rev->ns->revision)))
return -ERESTARTSYS; return -ERESTARTSYS;
mutex_lock(&rev->ns->lock); mutex_lock_nested(&rev->ns->lock, rev->ns->level);
} }
avail = sprintf(buffer, "%ld\n", rev->ns->revision); avail = sprintf(buffer, "%ld\n", rev->ns->revision);
...@@ -577,7 +577,7 @@ static unsigned int ns_revision_poll(struct file *file, poll_table *pt) ...@@ -577,7 +577,7 @@ static unsigned int ns_revision_poll(struct file *file, poll_table *pt)
unsigned int mask = 0; unsigned int mask = 0;
if (rev) { if (rev) {
mutex_lock(&rev->ns->lock); mutex_lock_nested(&rev->ns->lock, rev->ns->level);
poll_wait(file, &rev->ns->wait, pt); poll_wait(file, &rev->ns->wait, pt);
if (rev->last_read < rev->ns->revision) if (rev->last_read < rev->ns->revision)
mask |= POLLIN | POLLRDNORM; mask |= POLLIN | POLLRDNORM;
...@@ -1643,7 +1643,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode) ...@@ -1643,7 +1643,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode)
*/ */
inode_unlock(dir); inode_unlock(dir);
error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count); error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count);
mutex_lock(&parent->lock); mutex_lock_nested(&parent->lock, parent->level);
inode_lock_nested(dir, I_MUTEX_PARENT); inode_lock_nested(dir, I_MUTEX_PARENT);
if (error) if (error)
goto out; goto out;
...@@ -1692,7 +1692,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry) ...@@ -1692,7 +1692,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry)
inode_unlock(dir); inode_unlock(dir);
inode_unlock(dentry->d_inode); inode_unlock(dentry->d_inode);
mutex_lock(&parent->lock); mutex_lock_nested(&parent->lock, parent->level);
ns = aa_get_ns(__aa_findn_ns(&parent->sub_ns, dentry->d_name.name, ns = aa_get_ns(__aa_findn_ns(&parent->sub_ns, dentry->d_name.name,
dentry->d_name.len)); dentry->d_name.len));
if (!ns) { if (!ns) {
...@@ -1747,7 +1747,7 @@ void __aafs_ns_rmdir(struct aa_ns *ns) ...@@ -1747,7 +1747,7 @@ void __aafs_ns_rmdir(struct aa_ns *ns)
__aafs_profile_rmdir(child); __aafs_profile_rmdir(child);
list_for_each_entry(sub, &ns->sub_ns, base.list) { list_for_each_entry(sub, &ns->sub_ns, base.list) {
mutex_lock(&sub->lock); mutex_lock_nested(&sub->lock, sub->level);
__aafs_ns_rmdir(sub); __aafs_ns_rmdir(sub);
mutex_unlock(&sub->lock); mutex_unlock(&sub->lock);
} }
...@@ -1877,7 +1877,7 @@ int __aafs_ns_mkdir(struct aa_ns *ns, struct dentry *parent, const char *name, ...@@ -1877,7 +1877,7 @@ int __aafs_ns_mkdir(struct aa_ns *ns, struct dentry *parent, const char *name,
/* subnamespaces */ /* subnamespaces */
list_for_each_entry(sub, &ns->sub_ns, base.list) { list_for_each_entry(sub, &ns->sub_ns, base.list) {
mutex_lock(&sub->lock); mutex_lock_nested(&sub->lock, sub->level);
error = __aafs_ns_mkdir(sub, ns_subns_dir(ns), NULL, NULL); error = __aafs_ns_mkdir(sub, ns_subns_dir(ns), NULL, NULL);
mutex_unlock(&sub->lock); mutex_unlock(&sub->lock);
if (error) if (error)
...@@ -1921,7 +1921,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns) ...@@ -1921,7 +1921,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns)
/* is next namespace a child */ /* is next namespace a child */
if (!list_empty(&ns->sub_ns)) { if (!list_empty(&ns->sub_ns)) {
next = list_first_entry(&ns->sub_ns, typeof(*ns), base.list); next = list_first_entry(&ns->sub_ns, typeof(*ns), base.list);
mutex_lock(&next->lock); mutex_lock_nested(&next->lock, next->level);
return next; return next;
} }
...@@ -1931,7 +1931,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns) ...@@ -1931,7 +1931,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns)
mutex_unlock(&ns->lock); mutex_unlock(&ns->lock);
next = list_next_entry(ns, base.list); next = list_next_entry(ns, base.list);
if (!list_entry_is_head(next, &parent->sub_ns, base.list)) { if (!list_entry_is_head(next, &parent->sub_ns, base.list)) {
mutex_lock(&next->lock); mutex_lock_nested(&next->lock, next->level);
return next; return next;
} }
ns = parent; ns = parent;
...@@ -2039,7 +2039,7 @@ static void *p_start(struct seq_file *f, loff_t *pos) ...@@ -2039,7 +2039,7 @@ static void *p_start(struct seq_file *f, loff_t *pos)
f->private = root; f->private = root;
/* find the first profile */ /* find the first profile */
mutex_lock(&root->lock); mutex_lock_nested(&root->lock, root->level);
profile = __first_profile(root, root); profile = __first_profile(root, root);
/* skip to position */ /* skip to position */
...@@ -2491,7 +2491,7 @@ static int __init aa_create_aafs(void) ...@@ -2491,7 +2491,7 @@ static int __init aa_create_aafs(void)
ns_subrevision(root_ns) = dent; ns_subrevision(root_ns) = dent;
/* policy tree referenced by magic policy symlink */ /* policy tree referenced by magic policy symlink */
mutex_lock(&root_ns->lock); mutex_lock_nested(&root_ns->lock, root_ns->level);
error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy", error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy",
aafs_mnt->mnt_root); aafs_mnt->mnt_root);
mutex_unlock(&root_ns->lock); mutex_unlock(&root_ns->lock);
......
...@@ -2115,7 +2115,7 @@ void __aa_labelset_update_subtree(struct aa_ns *ns) ...@@ -2115,7 +2115,7 @@ void __aa_labelset_update_subtree(struct aa_ns *ns)
__labelset_update(ns); __labelset_update(ns);
list_for_each_entry(child, &ns->sub_ns, base.list) { list_for_each_entry(child, &ns->sub_ns, base.list) {
mutex_lock(&child->lock); mutex_lock_nested(&child->lock, child->level);
__aa_labelset_update_subtree(child); __aa_labelset_update_subtree(child);
mutex_unlock(&child->lock); mutex_unlock(&child->lock);
} }
......
...@@ -545,7 +545,7 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, bool hat, ...@@ -545,7 +545,7 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, bool hat,
profile->file.dfa = aa_get_dfa(nulldfa); profile->file.dfa = aa_get_dfa(nulldfa);
profile->policy.dfa = aa_get_dfa(nulldfa); profile->policy.dfa = aa_get_dfa(nulldfa);
mutex_lock(&profile->ns->lock); mutex_lock_nested(&profile->ns->lock, profile->ns->level);
p = __find_child(&parent->base.profiles, bname); p = __find_child(&parent->base.profiles, bname);
if (p) { if (p) {
aa_free_profile(profile); aa_free_profile(profile);
...@@ -906,7 +906,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, ...@@ -906,7 +906,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,
} else } else
ns = aa_get_ns(policy_ns ? policy_ns : labels_ns(label)); ns = aa_get_ns(policy_ns ? policy_ns : labels_ns(label));
mutex_lock(&ns->lock); mutex_lock_nested(&ns->lock, ns->level);
/* check for duplicate rawdata blobs: space and file dedup */ /* check for duplicate rawdata blobs: space and file dedup */
list_for_each_entry(rawdata_ent, &ns->rawdata_list, list) { list_for_each_entry(rawdata_ent, &ns->rawdata_list, list) {
if (aa_rawdata_eq(rawdata_ent, udata)) { if (aa_rawdata_eq(rawdata_ent, udata)) {
...@@ -1117,13 +1117,13 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj, ...@@ -1117,13 +1117,13 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj,
if (!name) { if (!name) {
/* remove namespace - can only happen if fqname[0] == ':' */ /* remove namespace - can only happen if fqname[0] == ':' */
mutex_lock(&ns->parent->lock); mutex_lock_nested(&ns->parent->lock, ns->level);
__aa_remove_ns(ns); __aa_remove_ns(ns);
__aa_bump_ns_revision(ns); __aa_bump_ns_revision(ns);
mutex_unlock(&ns->parent->lock); mutex_unlock(&ns->parent->lock);
} else { } else {
/* remove profile */ /* remove profile */
mutex_lock(&ns->lock); mutex_lock_nested(&ns->lock, ns->level);
profile = aa_get_profile(__lookup_profile(&ns->base, name)); profile = aa_get_profile(__lookup_profile(&ns->base, name));
if (!profile) { if (!profile) {
error = -ENOENT; error = -ENOENT;
......
...@@ -256,7 +256,8 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, ...@@ -256,7 +256,8 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name,
ns = alloc_ns(parent->base.hname, name); ns = alloc_ns(parent->base.hname, name);
if (!ns) if (!ns)
return NULL; return NULL;
mutex_lock(&ns->lock); ns->level = parent->level + 1;
mutex_lock_nested(&ns->lock, ns->level);
error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir); error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir);
if (error) { if (error) {
AA_ERROR("Failed to create interface for ns %s\n", AA_ERROR("Failed to create interface for ns %s\n",
...@@ -266,7 +267,6 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, ...@@ -266,7 +267,6 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name,
return ERR_PTR(error); return ERR_PTR(error);
} }
ns->parent = aa_get_ns(parent); ns->parent = aa_get_ns(parent);
ns->level = parent->level + 1;
list_add_rcu(&ns->base.list, &parent->sub_ns); list_add_rcu(&ns->base.list, &parent->sub_ns);
/* add list ref */ /* add list ref */
aa_get_ns(ns); aa_get_ns(ns);
...@@ -313,7 +313,7 @@ struct aa_ns *aa_prepare_ns(struct aa_ns *parent, const char *name) ...@@ -313,7 +313,7 @@ struct aa_ns *aa_prepare_ns(struct aa_ns *parent, const char *name)
{ {
struct aa_ns *ns; struct aa_ns *ns;
mutex_lock(&parent->lock); mutex_lock_nested(&parent->lock, parent->level);
/* try and find the specified ns and if it doesn't exist create it */ /* try and find the specified ns and if it doesn't exist create it */
/* released by caller */ /* released by caller */
ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name)); ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name));
...@@ -336,7 +336,7 @@ static void destroy_ns(struct aa_ns *ns) ...@@ -336,7 +336,7 @@ static void destroy_ns(struct aa_ns *ns)
if (!ns) if (!ns)
return; return;
mutex_lock(&ns->lock); mutex_lock_nested(&ns->lock, ns->level);
/* release all profiles in this namespace */ /* release all profiles in this namespace */
__aa_profile_list_release(&ns->base.profiles); __aa_profile_list_release(&ns->base.profiles);
......
...@@ -157,7 +157,7 @@ static void do_loaddata_free(struct work_struct *work) ...@@ -157,7 +157,7 @@ static void do_loaddata_free(struct work_struct *work)
struct aa_ns *ns = aa_get_ns(d->ns); struct aa_ns *ns = aa_get_ns(d->ns);
if (ns) { if (ns) {
mutex_lock(&ns->lock); mutex_lock_nested(&ns->lock, ns->level);
__aa_fs_remove_rawdata(d); __aa_fs_remove_rawdata(d);
mutex_unlock(&ns->lock); mutex_unlock(&ns->lock);
aa_put_ns(ns); aa_put_ns(ns);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment