Re: Linux 5.18-rc4

From: Matthew Wilcox
Date: Mon Jun 06 2022 - 16:25:52 EST


On Mon, Jun 06, 2022 at 12:19:36PM -0700, John Johansen wrote:
> > 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.


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);
}