Re: Linux 5.18-rc4
From: John Johansen <john.johansen@canonical.com>
Date: 2022-06-06 21:07:25
Also in:
linux-fsdevel, linux-mm, lkml
Possibly related (same subject, not in this thread)
- 2022-04-27 · Re: Linux 5.18-rc4 · Linus Torvalds <torvalds@linux-foundation.org>
On 6/6/22 13:23, Matthew Wilcox wrote:
On Mon, Jun 06, 2022 at 12:19:36PM -0700, John Johansen wrote:quoted
quoted
I suspect that part is that both Apparmor and IPC use the idr local lock.bingo, apparmor moved its secids allocation from a custom radix tree to idr in 99cc45e48678 apparmor: Use an IDR to allocate apparmor secids and ipc is using the idr for its id allocation as well I can easily lift the secid() allocation out of the ctx->lock but that would still leave it happening under the file_lock and not fix the problem. I think the quick solution would be for apparmor to stop using idr, reverting back at least temporarily to the custom radix tree.How about moving forward to the XArray that doesn't use that horrid prealloc gunk? Compile tested only.
I'm not very familiar with XArray but it does seem like a good fit. We do try to keep the secid allocation dense, ideally no holes. Wrt the current locking issue I want to hear what Thomas has to say. Regardless I am looking into whether we should just switch to XArrays going forward.
quoted hunk ↗ jump to hunk
diff --git a/security/apparmor/include/secid.h b/security/apparmor/include/secid.h index 48ff1ddecad5..278dff5ecd1f 100644 --- a/security/apparmor/include/secid.h +++ b/security/apparmor/include/secid.h@@ -31,6 +31,4 @@ int aa_alloc_secid(struct aa_label *label, gfp_t gfp); void aa_free_secid(u32 secid); void aa_secid_update(u32 secid, struct aa_label *label); -void aa_secids_init(void); - #endif /* __AA_SECID_H */diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 900bc540656a..9dfb4e4631da 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c@@ -1857,8 +1857,6 @@ static int __init apparmor_init(void) { int error; - aa_secids_init(); - error = aa_setup_dfa_engine(); if (error) { AA_ERROR("Unable to setup dfa engine\n");diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c index ce545f99259e..3b08942db1f6 100644 --- a/security/apparmor/secid.c +++ b/security/apparmor/secid.c@@ -13,9 +13,9 @@ #include <linux/errno.h> #include <linux/err.h> #include <linux/gfp.h> -#include <linux/idr.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/xarray.h> #include "include/cred.h" #include "include/lib.h"@@ -29,8 +29,7 @@ */ #define AA_FIRST_SECID 2 -static DEFINE_IDR(aa_secids); -static DEFINE_SPINLOCK(secid_lock); +static DEFINE_XARRAY_FLAGS(aa_secids, XA_FLAGS_LOCK_IRQ | XA_FLAGS_TRACK_FREE); /* * TODO: allow policy to reserve a secid range?@@ -47,9 +46,9 @@ void aa_secid_update(u32 secid, struct aa_label *label) { unsigned long flags; - spin_lock_irqsave(&secid_lock, flags); - idr_replace(&aa_secids, label, secid); - spin_unlock_irqrestore(&secid_lock, flags); + xa_lock_irqsave(&aa_secids, flags); + __xa_store(&aa_secids, secid, label, 0); + xa_unlock_irqrestore(&aa_secids, flags); } /**@@ -58,13 +57,7 @@ void aa_secid_update(u32 secid, struct aa_label *label) */ struct aa_label *aa_secid_to_label(u32 secid) { - struct aa_label *label; - - rcu_read_lock(); - label = idr_find(&aa_secids, secid); - rcu_read_unlock(); - - return label; + return xa_load(&aa_secids, secid); } int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)@@ -126,19 +119,16 @@ int aa_alloc_secid(struct aa_label *label, gfp_t gfp) unsigned long flags; int ret; - idr_preload(gfp); - spin_lock_irqsave(&secid_lock, flags); - ret = idr_alloc(&aa_secids, label, AA_FIRST_SECID, 0, GFP_ATOMIC); - spin_unlock_irqrestore(&secid_lock, flags); - idr_preload_end(); + xa_lock_irqsave(&aa_secids, flags); + ret = __xa_alloc(&aa_secids, &label->secid, label, + XA_LIMIT(AA_FIRST_SECID, INT_MAX), gfp); + xa_unlock_irqrestore(&aa_secids, flags); if (ret < 0) { label->secid = AA_SECID_INVALID; return ret; } - AA_BUG(ret == AA_SECID_INVALID); - label->secid = ret; return 0; }@@ -150,12 +140,7 @@ void aa_free_secid(u32 secid) { unsigned long flags; - spin_lock_irqsave(&secid_lock, flags); - idr_remove(&aa_secids, secid); - spin_unlock_irqrestore(&secid_lock, flags); -} - -void aa_secids_init(void) -{ - idr_init_base(&aa_secids, AA_FIRST_SECID); + xa_lock_irqsave(&aa_secids, flags); + __xa_erase(&aa_secids, secid); + xa_unlock_irqrestore(&aa_secids, flags); }