Skip to content

Commit feb3c76

Browse files
jrjohansenjrjohansen
committed
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+ Freescale#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] Freescale#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+ Freescale#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: 73688d1 ("apparmor: refactor prepare_ns() and make usable from different views") Signed-off-by: John Johansen <[email protected]>
1 parent 5d7c44e commit feb3c76

5 files changed

Lines changed: 21 additions & 21 deletions

File tree

‎security/apparmor/apparmorfs.c‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -533,7 +533,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf,
533533
long last_read;
534534
int avail;
535535

536-
mutex_lock(&rev->ns->lock);
536+
mutex_lock_nested(&rev->ns->lock, rev->ns->level);
537537
last_read = rev->last_read;
538538
if (last_read == rev->ns->revision) {
539539
mutex_unlock(&rev->ns->lock);
@@ -543,7 +543,7 @@ static ssize_t ns_revision_read(struct file *file, char __user *buf,
543543
last_read !=
544544
READ_ONCE(rev->ns->revision)))
545545
return -ERESTARTSYS;
546-
mutex_lock(&rev->ns->lock);
546+
mutex_lock_nested(&rev->ns->lock, rev->ns->level);
547547
}
548548

549549
avail = sprintf(buffer, "%ld\n", rev->ns->revision);
@@ -577,7 +577,7 @@ static unsigned int ns_revision_poll(struct file *file, poll_table *pt)
577577
unsigned int mask = 0;
578578

579579
if (rev) {
580-
mutex_lock(&rev->ns->lock);
580+
mutex_lock_nested(&rev->ns->lock, rev->ns->level);
581581
poll_wait(file, &rev->ns->wait, pt);
582582
if (rev->last_read < rev->ns->revision)
583583
mask |= POLLIN | POLLRDNORM;
@@ -1643,7 +1643,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode)
16431643
*/
16441644
inode_unlock(dir);
16451645
error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count);
1646-
mutex_lock(&parent->lock);
1646+
mutex_lock_nested(&parent->lock, parent->level);
16471647
inode_lock_nested(dir, I_MUTEX_PARENT);
16481648
if (error)
16491649
goto out;
@@ -1692,7 +1692,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry)
16921692
inode_unlock(dir);
16931693
inode_unlock(dentry->d_inode);
16941694

1695-
mutex_lock(&parent->lock);
1695+
mutex_lock_nested(&parent->lock, parent->level);
16961696
ns = aa_get_ns(__aa_findn_ns(&parent->sub_ns, dentry->d_name.name,
16971697
dentry->d_name.len));
16981698
if (!ns) {
@@ -1747,7 +1747,7 @@ void __aafs_ns_rmdir(struct aa_ns *ns)
17471747
__aafs_profile_rmdir(child);
17481748

17491749
list_for_each_entry(sub, &ns->sub_ns, base.list) {
1750-
mutex_lock(&sub->lock);
1750+
mutex_lock_nested(&sub->lock, sub->level);
17511751
__aafs_ns_rmdir(sub);
17521752
mutex_unlock(&sub->lock);
17531753
}
@@ -1877,7 +1877,7 @@ int __aafs_ns_mkdir(struct aa_ns *ns, struct dentry *parent, const char *name,
18771877

18781878
/* subnamespaces */
18791879
list_for_each_entry(sub, &ns->sub_ns, base.list) {
1880-
mutex_lock(&sub->lock);
1880+
mutex_lock_nested(&sub->lock, sub->level);
18811881
error = __aafs_ns_mkdir(sub, ns_subns_dir(ns), NULL, NULL);
18821882
mutex_unlock(&sub->lock);
18831883
if (error)
@@ -1921,7 +1921,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns)
19211921
/* is next namespace a child */
19221922
if (!list_empty(&ns->sub_ns)) {
19231923
next = list_first_entry(&ns->sub_ns, typeof(*ns), base.list);
1924-
mutex_lock(&next->lock);
1924+
mutex_lock_nested(&next->lock, next->level);
19251925
return next;
19261926
}
19271927

@@ -1931,7 +1931,7 @@ static struct aa_ns *__next_ns(struct aa_ns *root, struct aa_ns *ns)
19311931
mutex_unlock(&ns->lock);
19321932
next = list_next_entry(ns, base.list);
19331933
if (!list_entry_is_head(next, &parent->sub_ns, base.list)) {
1934-
mutex_lock(&next->lock);
1934+
mutex_lock_nested(&next->lock, next->level);
19351935
return next;
19361936
}
19371937
ns = parent;
@@ -2039,7 +2039,7 @@ static void *p_start(struct seq_file *f, loff_t *pos)
20392039
f->private = root;
20402040

20412041
/* find the first profile */
2042-
mutex_lock(&root->lock);
2042+
mutex_lock_nested(&root->lock, root->level);
20432043
profile = __first_profile(root, root);
20442044

20452045
/* skip to position */
@@ -2491,7 +2491,7 @@ static int __init aa_create_aafs(void)
24912491
ns_subrevision(root_ns) = dent;
24922492

24932493
/* policy tree referenced by magic policy symlink */
2494-
mutex_lock(&root_ns->lock);
2494+
mutex_lock_nested(&root_ns->lock, root_ns->level);
24952495
error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy",
24962496
aafs_mnt->mnt_root);
24972497
mutex_unlock(&root_ns->lock);

‎security/apparmor/label.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2115,7 +2115,7 @@ void __aa_labelset_update_subtree(struct aa_ns *ns)
21152115
__labelset_update(ns);
21162116

21172117
list_for_each_entry(child, &ns->sub_ns, base.list) {
2118-
mutex_lock(&child->lock);
2118+
mutex_lock_nested(&child->lock, child->level);
21192119
__aa_labelset_update_subtree(child);
21202120
mutex_unlock(&child->lock);
21212121
}

‎security/apparmor/policy.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, bool hat,
545545
profile->file.dfa = aa_get_dfa(nulldfa);
546546
profile->policy.dfa = aa_get_dfa(nulldfa);
547547

548-
mutex_lock(&profile->ns->lock);
548+
mutex_lock_nested(&profile->ns->lock, profile->ns->level);
549549
p = __find_child(&parent->base.profiles, bname);
550550
if (p) {
551551
aa_free_profile(profile);
@@ -906,7 +906,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,
906906
} else
907907
ns = aa_get_ns(policy_ns ? policy_ns : labels_ns(label));
908908

909-
mutex_lock(&ns->lock);
909+
mutex_lock_nested(&ns->lock, ns->level);
910910
/* check for duplicate rawdata blobs: space and file dedup */
911911
list_for_each_entry(rawdata_ent, &ns->rawdata_list, list) {
912912
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,
11171117

11181118
if (!name) {
11191119
/* remove namespace - can only happen if fqname[0] == ':' */
1120-
mutex_lock(&ns->parent->lock);
1120+
mutex_lock_nested(&ns->parent->lock, ns->level);
11211121
__aa_remove_ns(ns);
11221122
__aa_bump_ns_revision(ns);
11231123
mutex_unlock(&ns->parent->lock);
11241124
} else {
11251125
/* remove profile */
1126-
mutex_lock(&ns->lock);
1126+
mutex_lock_nested(&ns->lock, ns->level);
11271127
profile = aa_get_profile(__lookup_profile(&ns->base, name));
11281128
if (!profile) {
11291129
error = -ENOENT;

‎security/apparmor/policy_ns.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,8 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name,
256256
ns = alloc_ns(parent->base.hname, name);
257257
if (!ns)
258258
return NULL;
259-
mutex_lock(&ns->lock);
259+
ns->level = parent->level + 1;
260+
mutex_lock_nested(&ns->lock, ns->level);
260261
error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir);
261262
if (error) {
262263
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,
266267
return ERR_PTR(error);
267268
}
268269
ns->parent = aa_get_ns(parent);
269-
ns->level = parent->level + 1;
270270
list_add_rcu(&ns->base.list, &parent->sub_ns);
271271
/* add list ref */
272272
aa_get_ns(ns);
@@ -313,7 +313,7 @@ struct aa_ns *aa_prepare_ns(struct aa_ns *parent, const char *name)
313313
{
314314
struct aa_ns *ns;
315315

316-
mutex_lock(&parent->lock);
316+
mutex_lock_nested(&parent->lock, parent->level);
317317
/* try and find the specified ns and if it doesn't exist create it */
318318
/* released by caller */
319319
ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name));
@@ -336,7 +336,7 @@ static void destroy_ns(struct aa_ns *ns)
336336
if (!ns)
337337
return;
338338

339-
mutex_lock(&ns->lock);
339+
mutex_lock_nested(&ns->lock, ns->level);
340340
/* release all profiles in this namespace */
341341
__aa_profile_list_release(&ns->base.profiles);
342342

‎security/apparmor/policy_unpack.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ static void do_loaddata_free(struct work_struct *work)
157157
struct aa_ns *ns = aa_get_ns(d->ns);
158158

159159
if (ns) {
160-
mutex_lock(&ns->lock);
160+
mutex_lock_nested(&ns->lock, ns->level);
161161
__aa_fs_remove_rawdata(d);
162162
mutex_unlock(&ns->lock);
163163
aa_put_ns(ns);

0 commit comments

Comments
 (0)