diff options
author | John Johansen <john.johansen@canonical.com> | 2017-11-21 02:24:09 -0500 |
---|---|---|
committer | John Johansen <john.johansen@canonical.com> | 2017-11-21 05:17:16 -0500 |
commit | feb3c766a3ab32d233aaff7db13afd9ba5bc142d (patch) | |
tree | 59b48447feb2bf23a53df1b2ead7d02fd440c957 | |
parent | 5d7c44ef5e4f0149c9fb99faeae41e930485a1ec (diff) |
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: 73688d1ed0b8 ("apparmor: refactor prepare_ns() and make usable from different views")
Signed-off-by: John Johansen <john.johansen@canonical.com>
-rw-r--r-- | security/apparmor/apparmorfs.c | 22 | ||||
-rw-r--r-- | security/apparmor/label.c | 2 | ||||
-rw-r--r-- | security/apparmor/policy.c | 8 | ||||
-rw-r--r-- | security/apparmor/policy_ns.c | 8 | ||||
-rw-r--r-- | security/apparmor/policy_unpack.c | 2 |
5 files changed, 21 insertions, 21 deletions
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index caaf51dda648..8542e9a55e1b 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c | |||
@@ -533,7 +533,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf, | |||
533 | long last_read; | 533 | long last_read; |
534 | int avail; | 534 | int avail; |
535 | 535 | ||
536 | mutex_lock(&rev->ns->lock); | 536 | mutex_lock_nested(&rev->ns->lock, rev->ns->level); |
537 | last_read = rev->last_read; | 537 | last_read = rev->last_read; |
538 | if (last_read == rev->ns->revision) { | 538 | if (last_read == rev->ns->revision) { |
539 | mutex_unlock(&rev->ns->lock); | 539 | mutex_unlock(&rev->ns->lock); |
@@ -543,7 +543,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf, | |||
543 | last_read != | 543 | last_read != |
544 | READ_ONCE(rev->ns->revision))) | 544 | READ_ONCE(rev->ns->revision))) |
545 | return -ERESTARTSYS; | 545 | return -ERESTARTSYS; |
546 | mutex_lock(&rev->ns->lock); | 546 | mutex_lock_nested(&rev->ns->lock, rev->ns->level); |
547 | } | 547 | } |
548 | 548 | ||
549 | avail = sprintf(buffer, "%ld\n", rev->ns->revision); | 549 | 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 | unsigned int mask = 0; | 577 | unsigned int mask = 0; |
578 | 578 | ||
579 | if (rev) { | 579 | if (rev) { |
580 | mutex_lock(&rev->ns->lock); | 580 | mutex_lock_nested(&rev->ns->lock, rev->ns->level); |
581 | poll_wait(file, &rev->ns->wait, pt); | 581 | poll_wait(file, &rev->ns->wait, pt); |
582 | if (rev->last_read < rev->ns->revision) | 582 | if (rev->last_read < rev->ns->revision) |
583 | mask |= POLLIN | POLLRDNORM; | 583 | mask |= POLLIN | POLLRDNORM; |
@@ -1643,7 +1643,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode) | |||
1643 | */ | 1643 | */ |
1644 | inode_unlock(dir); | 1644 | inode_unlock(dir); |
1645 | error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count); | 1645 | error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count); |
1646 | mutex_lock(&parent->lock); | 1646 | mutex_lock_nested(&parent->lock, parent->level); |
1647 | inode_lock_nested(dir, I_MUTEX_PARENT); | 1647 | inode_lock_nested(dir, I_MUTEX_PARENT); |
1648 | if (error) | 1648 | if (error) |
1649 | goto out; | 1649 | goto out; |
@@ -1692,7 +1692,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry) | |||
1692 | inode_unlock(dir); | 1692 | inode_unlock(dir); |
1693 | inode_unlock(dentry->d_inode); | 1693 | inode_unlock(dentry->d_inode); |
1694 | 1694 | ||
1695 | mutex_lock(&parent->lock); | 1695 | mutex_lock_nested(&parent->lock, parent->level); |
1696 | ns = aa_get_ns(__aa_findn_ns(&parent->sub_ns, dentry->d_name.name, | 1696 | ns = aa_get_ns(__aa_findn_ns(&parent->sub_ns, dentry->d_name.name, |
1697 | dentry->d_name.len)); | 1697 | dentry->d_name.len)); |
1698 | if (!ns) { | 1698 | if (!ns) { |
@@ -1747,7 +1747,7 @@ void __aafs_ns_rmdir(struct aa_ns *ns) | |||
1747 | __aafs_profile_rmdir(child); | 1747 | __aafs_profile_rmdir(child); |
1748 | 1748 | ||
1749 | list_for_each_entry(sub, &ns->sub_ns, base.list) { | 1749 | list_for_each_entry(sub, &ns->sub_ns, base.list) { |
1750 | mutex_lock(&sub->lock); | 1750 | mutex_lock_nested(&sub->lock, sub->level); |
1751 | __aafs_ns_rmdir(sub); | 1751 | __aafs_ns_rmdir(sub); |
1752 | mutex_unlock(&sub->lock); | 1752 | mutex_unlock(&sub->lock); |
1753 | } | 1753 | } |
@@ -1877,7 +1877,7 @@ int __aafs_ns_mkdir(struct aa_ns *ns, struct dentry *parent, const char *name, | |||
1877 | 1877 | ||
1878 | /* subnamespaces */ | 1878 | /* subnamespaces */ |
1879 | list_for_each_entry(sub, &ns->sub_ns, base.list) { | 1879 | list_for_each_entry(sub, &ns->sub_ns, base.list) { |
1880 | mutex_lock(&sub->lock); | 1880 | mutex_lock_nested(&sub->lock, sub->level); |
1881 | error = __aafs_ns_mkdir(sub, ns_subns_dir(ns), NULL, NULL); | 1881 | error = __aafs_ns_mkdir(sub, ns_subns_dir(ns), NULL, NULL); |
1882 | mutex_unlock(&sub->lock); | 1882 | mutex_unlock(&sub->lock); |
1883 | if (error) | 1883 | if (error) |
@@ -1921,7 +1921,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns) | |||
1921 | /* is next namespace a child */ | 1921 | /* is next namespace a child */ |
1922 | if (!list_empty(&ns->sub_ns)) { | 1922 | if (!list_empty(&ns->sub_ns)) { |
1923 | next = list_first_entry(&ns->sub_ns, typeof(*ns), base.list); | 1923 | next = list_first_entry(&ns->sub_ns, typeof(*ns), base.list); |
1924 | mutex_lock(&next->lock); | 1924 | mutex_lock_nested(&next->lock, next->level); |
1925 | return next; | 1925 | return next; |
1926 | } | 1926 | } |
1927 | 1927 | ||
@@ -1931,7 +1931,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns) | |||
1931 | mutex_unlock(&ns->lock); | 1931 | mutex_unlock(&ns->lock); |
1932 | next = list_next_entry(ns, base.list); | 1932 | next = list_next_entry(ns, base.list); |
1933 | if (!list_entry_is_head(next, &parent->sub_ns, base.list)) { | 1933 | if (!list_entry_is_head(next, &parent->sub_ns, base.list)) { |
1934 | mutex_lock(&next->lock); | 1934 | mutex_lock_nested(&next->lock, next->level); |
1935 | return next; | 1935 | return next; |
1936 | } | 1936 | } |
1937 | ns = parent; | 1937 | ns = parent; |
@@ -2039,7 +2039,7 @@ static void *p_start(struct seq_file *f, loff_t *pos) | |||
2039 | f->private = root; | 2039 | f->private = root; |
2040 | 2040 | ||
2041 | /* find the first profile */ | 2041 | /* find the first profile */ |
2042 | mutex_lock(&root->lock); | 2042 | mutex_lock_nested(&root->lock, root->level); |
2043 | profile = __first_profile(root, root); | 2043 | profile = __first_profile(root, root); |
2044 | 2044 | ||
2045 | /* skip to position */ | 2045 | /* skip to position */ |
@@ -2491,7 +2491,7 @@ static int __init aa_create_aafs(void) | |||
2491 | ns_subrevision(root_ns) = dent; | 2491 | ns_subrevision(root_ns) = dent; |
2492 | 2492 | ||
2493 | /* policy tree referenced by magic policy symlink */ | 2493 | /* policy tree referenced by magic policy symlink */ |
2494 | mutex_lock(&root_ns->lock); | 2494 | mutex_lock_nested(&root_ns->lock, root_ns->level); |
2495 | error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy", | 2495 | error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy", |
2496 | aafs_mnt->mnt_root); | 2496 | aafs_mnt->mnt_root); |
2497 | mutex_unlock(&root_ns->lock); | 2497 | mutex_unlock(&root_ns->lock); |
diff --git a/security/apparmor/label.c b/security/apparmor/label.c index c5b99b954580..f5df9eaba4df 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c | |||
@@ -2115,7 +2115,7 @@ void __aa_labelset_update_subtree(struct aa_ns *ns) | |||
2115 | __labelset_update(ns); | 2115 | __labelset_update(ns); |
2116 | 2116 | ||
2117 | list_for_each_entry(child, &ns->sub_ns, base.list) { | 2117 | list_for_each_entry(child, &ns->sub_ns, base.list) { |
2118 | mutex_lock(&child->lock); | 2118 | mutex_lock_nested(&child->lock, child->level); |
2119 | __aa_labelset_update_subtree(child); | 2119 | __aa_labelset_update_subtree(child); |
2120 | mutex_unlock(&child->lock); | 2120 | mutex_unlock(&child->lock); |
2121 | } | 2121 | } |
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 586b249d3b46..b0b58848c248 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c | |||
@@ -545,7 +545,7 @@ name: | |||
545 | profile->file.dfa = aa_get_dfa(nulldfa); | 545 | profile->file.dfa = aa_get_dfa(nulldfa); |
546 | profile->policy.dfa = aa_get_dfa(nulldfa); | 546 | profile->policy.dfa = aa_get_dfa(nulldfa); |
547 | 547 | ||
548 | mutex_lock(&profile->ns->lock); | 548 | mutex_lock_nested(&profile->ns->lock, profile->ns->level); |
549 | p = __find_child(&parent->base.profiles, bname); | 549 | p = __find_child(&parent->base.profiles, bname); |
550 | if (p) { | 550 | if (p) { |
551 | aa_free_profile(profile); | 551 | aa_free_profile(profile); |
@@ -906,7 +906,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, | |||
906 | } else | 906 | } else |
907 | ns = aa_get_ns(policy_ns ? policy_ns : labels_ns(label)); | 907 | ns = aa_get_ns(policy_ns ? policy_ns : labels_ns(label)); |
908 | 908 | ||
909 | mutex_lock(&ns->lock); | 909 | mutex_lock_nested(&ns->lock, ns->level); |
910 | /* check for duplicate rawdata blobs: space and file dedup */ | 910 | /* check for duplicate rawdata blobs: space and file dedup */ |
911 | list_for_each_entry(rawdata_ent, &ns->rawdata_list, list) { | 911 | list_for_each_entry(rawdata_ent, &ns->rawdata_list, list) { |
912 | if (aa_rawdata_eq(rawdata_ent, udata)) { | 912 | 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 | 1117 | ||
1118 | if (!name) { | 1118 | if (!name) { |
1119 | /* remove namespace - can only happen if fqname[0] == ':' */ | 1119 | /* remove namespace - can only happen if fqname[0] == ':' */ |
1120 | mutex_lock(&ns->parent->lock); | 1120 | mutex_lock_nested(&ns->parent->lock, ns->level); |
1121 | __aa_remove_ns(ns); | 1121 | __aa_remove_ns(ns); |
1122 | __aa_bump_ns_revision(ns); | 1122 | __aa_bump_ns_revision(ns); |
1123 | mutex_unlock(&ns->parent->lock); | 1123 | mutex_unlock(&ns->parent->lock); |
1124 | } else { | 1124 | } else { |
1125 | /* remove profile */ | 1125 | /* remove profile */ |
1126 | mutex_lock(&ns->lock); | 1126 | mutex_lock_nested(&ns->lock, ns->level); |
1127 | profile = aa_get_profile(__lookup_profile(&ns->base, name)); | 1127 | profile = aa_get_profile(__lookup_profile(&ns->base, name)); |
1128 | if (!profile) { | 1128 | if (!profile) { |
1129 | error = -ENOENT; | 1129 | error = -ENOENT; |
diff --git a/security/apparmor/policy_ns.c b/security/apparmor/policy_ns.c index 62a3589c62ab..b1e629cba70b 100644 --- a/security/apparmor/policy_ns.c +++ b/security/apparmor/policy_ns.c | |||
@@ -256,7 +256,8 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, | |||
256 | ns = alloc_ns(parent->base.hname, name); | 256 | ns = alloc_ns(parent->base.hname, name); |
257 | if (!ns) | 257 | if (!ns) |
258 | return NULL; | 258 | return NULL; |
259 | mutex_lock(&ns->lock); | 259 | ns->level = parent->level + 1; |
260 | mutex_lock_nested(&ns->lock, ns->level); | ||
260 | error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir); | 261 | error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir); |
261 | if (error) { | 262 | if (error) { |
262 | AA_ERROR("Failed to create interface for ns %s\n", | 263 | 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 | return ERR_PTR(error); | 267 | return ERR_PTR(error); |
267 | } | 268 | } |
268 | ns->parent = aa_get_ns(parent); | 269 | ns->parent = aa_get_ns(parent); |
269 | ns->level = parent->level + 1; | ||
270 | list_add_rcu(&ns->base.list, &parent->sub_ns); | 270 | list_add_rcu(&ns->base.list, &parent->sub_ns); |
271 | /* add list ref */ | 271 | /* add list ref */ |
272 | aa_get_ns(ns); | 272 | aa_get_ns(ns); |
@@ -313,7 +313,7 @@ struct aa_ns *aa_prepare_ns(struct aa_ns *parent, const char *name) | |||
313 | { | 313 | { |
314 | struct aa_ns *ns; | 314 | struct aa_ns *ns; |
315 | 315 | ||
316 | mutex_lock(&parent->lock); | 316 | mutex_lock_nested(&parent->lock, parent->level); |
317 | /* try and find the specified ns and if it doesn't exist create it */ | 317 | /* try and find the specified ns and if it doesn't exist create it */ |
318 | /* released by caller */ | 318 | /* released by caller */ |
319 | ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name)); | 319 | ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name)); |
@@ -336,7 +336,7 @@ static void destroy_ns(struct aa_ns *ns) | |||
336 | if (!ns) | 336 | if (!ns) |
337 | return; | 337 | return; |
338 | 338 | ||
339 | mutex_lock(&ns->lock); | 339 | mutex_lock_nested(&ns->lock, ns->level); |
340 | /* release all profiles in this namespace */ | 340 | /* release all profiles in this namespace */ |
341 | __aa_profile_list_release(&ns->base.profiles); | 341 | __aa_profile_list_release(&ns->base.profiles); |
342 | 342 | ||
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 4ede87c30f8b..59a1a25b7d43 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c | |||
@@ -157,7 +157,7 @@ static void do_loaddata_free(struct work_struct *work) | |||
157 | struct aa_ns *ns = aa_get_ns(d->ns); | 157 | struct aa_ns *ns = aa_get_ns(d->ns); |
158 | 158 | ||
159 | if (ns) { | 159 | if (ns) { |
160 | mutex_lock(&ns->lock); | 160 | mutex_lock_nested(&ns->lock, ns->level); |
161 | __aa_fs_remove_rawdata(d); | 161 | __aa_fs_remove_rawdata(d); |
162 | mutex_unlock(&ns->lock); | 162 | mutex_unlock(&ns->lock); |
163 | aa_put_ns(ns); | 163 | aa_put_ns(ns); |