[apparmor] [PATCH 19/27] apparmor: convert profile lists to RCU based locking
John Johansen
john.johansen at canonical.com
Wed Nov 21 04:39:59 UTC 2012
signed-offby: John Johansen <john.johansen at canonical.com>
---
security/apparmor/domain.c | 8 +-
security/apparmor/include/policy.h | 17 +--
security/apparmor/policy.c | 199 +++++++++++++++++++-----------------
security/apparmor/policy_unpack.c | 5 +
4 files changed, 125 insertions(+), 104 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index b5deff1..9ed217a 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -144,7 +144,7 @@ static struct aa_profile *__attach_match(const char *name,
int len = 0;
struct aa_profile *profile, *candidate = NULL;
- list_for_each_entry(profile, head, base.list) {
+ list_for_each_entry_rcu(profile, head, base.list) {
if (profile->flags & PFLAG_NULL)
continue;
if (profile->xmatch && profile->xmatch_len > len) {
@@ -177,9 +177,9 @@ static struct aa_profile *find_attach(struct aa_namespace *ns,
{
struct aa_profile *profile;
- read_lock(&ns->lock);
+ rcu_read_lock();
profile = aa_get_profile(__attach_match(name, list));
- read_unlock(&ns->lock);
+ rcu_read_unlock();
return profile;
}
@@ -648,7 +648,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
hat = aa_find_child(root, hats[i]);
if (!hat) {
if (!COMPLAIN_MODE(root) || permtest) {
- if (list_empty(&root->base.profiles))
+ if (list_empty_rcu(&root->base.profiles))
error = -ECHILD;
else
error = -ENOENT;
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index aadcbf8..0d16d91 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -42,6 +42,7 @@ extern const char *const profile_mode_names[];
#define PROFILE_IS_HAT(_profile) ((_profile)->flags & PFLAG_HAT)
+#define list_empty_rcu(X) (list_empty(X) || (X)->prev == LIST_POISON2)
/*
* FIXME: currently need a clean way to replace and remove profiles as a
* set. It should be done at the namespace level.
@@ -75,6 +76,7 @@ struct aa_profile;
* @hname - The hierarchical name
* @count: reference count of the obj
* @list: list policy object is on
+ * @rcu: rcu head used when removing from @list
* @profiles: head of the profiles list contained in the object
*/
struct aa_policy {
@@ -83,6 +85,7 @@ struct aa_policy {
struct kref count;
struct list_head list;
struct list_head profiles;
+ struct rcu_head rcu;
};
/* struct aa_ns_acct - accounting of profiles in namespace
@@ -123,9 +126,9 @@ struct aa_ns_acct {
struct aa_namespace {
struct aa_policy base;
struct aa_namespace *parent;
- rwlock_t lock;
+ struct mutex lock;
struct aa_ns_acct acct;
- struct aa_profile *unconfined;
+ struct aa_profile __rcu *unconfined;
struct list_head sub_ns;
atomic_t uniq_null;
@@ -166,7 +169,7 @@ struct aa_policydb {
* attachments are determined by profile X transition rules.
*
* The @replacedby field is write protected by the profile lock. Reads
- * are assumed to be atomic, and are done without locking.
+ * are assumed to be atomic, and are done with rcu.
*
* Profiles have a hierarchy where hats and children profiles keep
* a reference to their parent.
@@ -177,10 +180,10 @@ struct aa_policydb {
*/
struct aa_profile {
struct aa_policy base;
- struct aa_profile *parent;
+ struct aa_profile __rcu *parent;
struct aa_namespace *ns;
- struct aa_profile *replacedby;
+ struct aa_profile __rcu *replacedby;
const char *rename;
struct aa_dfa *xmatch;
@@ -274,8 +277,8 @@ ssize_t aa_remove_profiles(char *name, size_t size);
*/
static inline struct aa_profile *aa_newest_version(struct aa_profile *profile)
{
- while (profile->replacedby)
- profile = profile->replacedby;
+ while (rcu_dereference(profile->replacedby))
+ profile = rcu_dereference(profile->replacedby);
return profile;
}
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index ea4653d..4d3f8ba 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -153,13 +153,13 @@ static bool policy_init(struct aa_policy *policy, const char *prefix,
static void policy_destroy(struct aa_policy *policy)
{
/* still contains profiles -- invalid */
- if (!list_empty(&policy->profiles)) {
+ if (!list_empty_rcu(&policy->profiles)) {
AA_ERROR("%s: internal error, "
"policy '%s' still contains profiles\n",
__func__, policy->name);
BUG();
}
- if (!list_empty(&policy->list)) {
+ if (!list_empty_rcu(&policy->list)) {
AA_ERROR("%s: internal error, policy '%s' still on list\n",
__func__, policy->name);
BUG();
@@ -174,7 +174,7 @@ static void policy_destroy(struct aa_policy *policy)
* @head: list to search (NOT NULL)
* @name: name to search for (NOT NULL)
*
- * Requires: correct locks for the @head list be held
+ * Requires: rcu_read_lock be held
*
* Returns: unrefcounted policy that match @name or NULL if not found
*/
@@ -182,7 +182,7 @@ static struct aa_policy *__policy_find(struct list_head *head, const char *name)
{
struct aa_policy *policy;
- list_for_each_entry(policy, head, list) {
+ list_for_each_entry_rcu(policy, head, list) {
if (!strcmp(policy->name, name))
return policy;
}
@@ -195,7 +195,7 @@ static struct aa_policy *__policy_find(struct list_head *head, const char *name)
* @str: string to search for (NOT NULL)
* @len: length of match required
*
- * Requires: correct locks for the @head list be held
+ * Requires: rcu_read_lock be held
*
* Returns: unrefcounted policy that match @str or NULL if not found
*
@@ -207,7 +207,7 @@ static struct aa_policy *__policy_strn_find(struct list_head *head,
{
struct aa_policy *policy;
- list_for_each_entry(policy, head, list) {
+ list_for_each_entry_rcu(policy, head, list) {
if (aa_strneq(policy->name, str, len))
return policy;
}
@@ -284,7 +284,7 @@ static struct aa_namespace *alloc_namespace(const char *prefix,
goto fail_ns;
INIT_LIST_HEAD(&ns->sub_ns);
- rwlock_init(&ns->lock);
+ mutex_init(&ns->lock);
/* released by free_namespace */
ns->unconfined = aa_alloc_profile("unconfined");
@@ -350,7 +350,7 @@ void aa_free_namespace_kref(struct kref *kref)
*
* Returns: unrefcounted namespace
*
- * Requires: ns lock be held
+ * Requires: rcu_read_lock be held
*/
static struct aa_namespace *__aa_find_namespace(struct list_head *head,
const char *name)
@@ -373,9 +373,9 @@ struct aa_namespace *aa_find_namespace(struct aa_namespace *root,
{
struct aa_namespace *ns = NULL;
- read_lock(&root->lock);
+ rcu_read_lock();
ns = aa_get_namespace(__aa_find_namespace(&root->sub_ns, name));
- read_unlock(&root->lock);
+ rcu_read_unlock();
return ns;
}
@@ -392,7 +392,7 @@ static struct aa_namespace *aa_prepare_namespace(const char *name)
root = aa_current_profile()->ns;
- write_lock(&root->lock);
+ mutex_lock(&root->lock);
/* if name isn't specified the profile is loaded to the current ns */
if (!name) {
@@ -404,32 +404,18 @@ static struct aa_namespace *aa_prepare_namespace(const char *name)
/* try and find the specified ns and if it doesn't exist create it */
/* released by caller */
ns = aa_get_namespace(__aa_find_namespace(&root->sub_ns, name));
- if (!ns) {
- /* namespace not found */
- struct aa_namespace *new_ns;
- write_unlock(&root->lock);
- new_ns = alloc_namespace(root->base.hname, name);
- if (!new_ns)
- return NULL;
- write_lock(&root->lock);
- /* test for race when new_ns was allocated */
- ns = __aa_find_namespace(&root->sub_ns, name);
- if (!ns) {
- /* add parent ref */
- new_ns->parent = aa_get_namespace(root);
-
- list_add(&new_ns->base.list, &root->sub_ns);
- /* add list ref */
- ns = aa_get_namespace(new_ns);
- } else {
- /* raced so free the new one */
- free_namespace(new_ns);
- /* get reference on namespace */
- aa_get_namespace(ns);
- }
+ if (!ns)
+ ns = alloc_namespace(root->base.hname, name);
+
+ if (ns) {
+ /* add parent ref */
+ ns->parent = aa_get_namespace(root);
+ list_add_rcu(&ns->base.list, &root->sub_ns);
+ /* add list ref */
+ aa_get_namespace(ns);
}
out:
- write_unlock(&root->lock);
+ mutex_unlock(&root->lock);
/* return ref */
return ns;
@@ -447,7 +433,7 @@ out:
static void __list_add_profile(struct list_head *list,
struct aa_profile *profile)
{
- list_add(&profile->base.list, list);
+ list_add_rcu(&profile->base.list, list);
/* get list reference */
aa_get_profile(profile);
}
@@ -466,10 +452,8 @@ static void __list_add_profile(struct list_head *list,
*/
static void __list_remove_profile(struct aa_profile *profile)
{
- list_del_init(&profile->base.list);
- if (!(profile->flags & PFLAG_NO_LIST_REF))
- /* release list reference */
- aa_put_profile(profile);
+ list_del_rcu(&profile->base.list);
+ aa_put_profile(profile);
}
/**
@@ -494,18 +478,26 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new)
else
policy = &old->ns->base;
- __list_add_profile(&policy->profiles, new);
- /* inherit children */
- list_for_each_entry_safe(child, tmp, &old->base.profiles, base.list) {
- aa_put_profile(child->parent);
- child->parent = aa_get_profile(new);
- /* list refcount transferred to @new*/
- list_move(&child->base.list, &new->base.profiles);
+ if (!list_empty_rcu(&old->base.profiles)) {
+ /* inherit children */
+ list_for_each_entry_safe(child, tmp, &old->base.profiles,
+ base.list) {
+ struct aa_profile *parent = child->parent;
+ rcu_assign_pointer(child->parent, aa_get_profile(new));
+ aa_put_profile(parent);
+ }
+ /* this really should be replaced with a something that doesn't
+ * use rcu sync as synching at each profile load will slow
+ * profile loads significantly
+ */
+ list_splice_init_rcu(&old->base.profiles, &new->base.profiles, synchronize_rcu);
}
/* released by free_profile */
- old->replacedby = aa_get_profile(new);
- __list_remove_profile(old);
+ rcu_assign_pointer(old->replacedby, aa_get_profile(new));
+ list_replace_rcu(&old->base.list, &new->base.list);
+ aa_get_profile(new);
+ aa_put_profile(old);
}
static void __profile_list_release(struct list_head *head);
@@ -520,8 +512,9 @@ static void __remove_profile(struct aa_profile *profile)
{
/* release any children lists first */
__profile_list_release(&profile->base.profiles);
- /* released by free_profile */
- profile->replacedby = aa_get_profile(profile->ns->unconfined);
+ /* replacedby released by free_profile */
+ rcu_assign_pointer(profile->replacedby,
+ aa_get_profile(profile->ns->unconfined));
__list_remove_profile(profile);
}
@@ -546,17 +539,40 @@ static void __ns_list_release(struct list_head *head);
*/
static void destroy_namespace(struct aa_namespace *ns)
{
+ struct aa_profile *unconfined;
+
if (!ns)
return;
- write_lock(&ns->lock);
+ mutex_lock(&ns->lock);
/* release all profiles in this namespace */
__profile_list_release(&ns->base.profiles);
/* release all sub namespaces */
__ns_list_release(&ns->sub_ns);
- write_unlock(&ns->lock);
+ unconfined = ns->unconfined;
+ /*
+ * break the ns, unconfined profile cyclic reference and forward
+ * all new unconfined profiles requests to the parent namespace
+ * This will result in all confined tasks that have a profile
+ * being removed, inheriting the parent->unconfined profile.
+ */
+ if (ns->parent)
+ ns->unconfined = aa_get_profile(ns->parent->unconfined);
+
+ /* release original ns->unconfined ref */
+ aa_put_profile(unconfined);
+
+ mutex_unlock(&ns->lock);
+}
+
+void aa_put_ns_rcu(struct rcu_head *head)
+{
+ struct aa_namespace *ns = container_of(head, struct aa_namespace,
+ base.rcu);
+ /* release ns->base.list ref */
+ aa_put_namespace(ns);
}
/**
@@ -567,26 +583,12 @@ static void destroy_namespace(struct aa_namespace *ns)
*/
static void __remove_namespace(struct aa_namespace *ns)
{
- struct aa_profile *unconfined = ns->unconfined;
-
/* remove ns from namespace list */
- list_del_init(&ns->base.list);
-
- /*
- * break the ns, unconfined profile cyclic reference and forward
- * all new unconfined profiles requests to the parent namespace
- * This will result in all confined tasks that have a profile
- * being removed, inheriting the parent->unconfined profile.
- */
- if (ns->parent)
- ns->unconfined = aa_get_profile(ns->parent->unconfined);
+ list_del_rcu(&ns->base.list);
destroy_namespace(ns);
- /* release original ns->unconfined ref */
- aa_put_profile(unconfined);
- /* release ns->base.list ref, from removal above */
- aa_put_namespace(ns);
+ call_rcu(&ns->base.rcu, aa_put_ns_rcu);
}
/**
@@ -650,7 +652,7 @@ static void free_profile(struct aa_profile *profile)
if (!profile)
return;
- if (!list_empty(&profile->base.list)) {
+ if (!list_empty_rcu(&profile->base.list)) {
AA_ERROR("%s: internal error, "
"profile '%s' still on ns list\n",
__func__, profile->base.name);
@@ -678,12 +680,12 @@ static void free_profile(struct aa_profile *profile)
* call (it looks like recursion, with free_profile calling
* free_profile) for each profile in the chain lp#1056078.
*/
- for (p = profile->replacedby; p; ) {
+ for (p = rcu_dereference(profile->replacedby); p; ) {
if (atomic_dec_and_test(&p->base.count.refcount)) {
/* no more refs on p, grab its replacedby */
- struct aa_profile *next = p->replacedby;
+ struct aa_profile *next = rcu_dereference(p->replacedby);
/* break the chain */
- p->replacedby = NULL;
+ rcu_assign_pointer(p->replacedby, NULL);
/* now free p, chain is broken */
free_profile(p);
@@ -697,6 +699,16 @@ static void free_profile(struct aa_profile *profile)
}
/**
+ * aa_free_profile_rcu - free aa_profile by rcu (called by aa_free_profile_kref)
+ * @head: rcu_head callback for freeing of a profile (NOT NULL)
+ */
+void aa_free_profile_rcu(struct rcu_head *head)
+{
+ struct aa_profile *p = container_of(head, struct aa_profile, base.rcu);
+ free_profile(p);
+}
+
+/**
* aa_free_profile_kref - free aa_profile by kref (called by aa_put_profile)
* @kr: kref callback for freeing of a profile (NOT NULL)
*/
@@ -704,8 +716,7 @@ void aa_free_profile_kref(struct kref *kref)
{
struct aa_profile *p = container_of(kref, struct aa_profile,
base.count);
-
- free_profile(p);
+ call_rcu(&p->base.rcu, aa_free_profile_rcu);
}
/**
@@ -772,9 +783,9 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat)
profile->parent = aa_get_profile(parent);
profile->ns = aa_get_namespace(parent->ns);
- write_lock(&profile->ns->lock);
+ mutex_lock(&profile->ns->lock);
__list_add_profile(&parent->base.profiles, profile);
- write_unlock(&profile->ns->lock);
+ mutex_unlock(&profile->ns->lock);
/* refcount released by caller */
return profile;
@@ -790,7 +801,7 @@ fail:
* @head: list to search (NOT NULL)
* @name: name of profile (NOT NULL)
*
- * Requires: ns lock protecting list be held
+ * Requires: rcu_read_lock be held
*
* Returns: unrefcounted profile ptr, or NULL if not found
*/
@@ -805,7 +816,7 @@ static struct aa_profile *__find_child(struct list_head *head, const char *name)
* @name: name of profile (NOT NULL)
* @len: length of @name substring to match
*
- * Requires: ns lock protecting list be held
+ * Requires: rcu_read_lock be held
*
* Returns: unrefcounted profile ptr, or NULL if not found
*/
@@ -826,9 +837,9 @@ struct aa_profile *aa_find_child(struct aa_profile *parent, const char *name)
{
struct aa_profile *profile;
- read_lock(&parent->ns->lock);
+ rcu_read_lock();
profile = aa_get_profile(__find_child(&parent->base.profiles, name));
- read_unlock(&parent->ns->lock);
+ rcu_read_unlock();
/* refcount released by caller */
return profile;
@@ -843,7 +854,7 @@ struct aa_profile *aa_find_child(struct aa_profile *parent, const char *name)
* that matches hname does not need to exist, in general this
* is used to load a new profile.
*
- * Requires: ns->lock be held
+ * Requires: rcu_read_lock be held
*
* Returns: unrefcounted policy or NULL if not found
*/
@@ -875,7 +886,7 @@ static struct aa_policy *__lookup_parent(struct aa_namespace *ns,
* @base: base list to start looking up profile name from (NOT NULL)
* @hname: hierarchical profile name (NOT NULL)
*
- * Requires: ns->lock be held
+ * Requires: rcu_read_lock be held
*
* Returns: unrefcounted profile pointer or NULL if not found
*
@@ -914,9 +925,9 @@ struct aa_profile *aa_lookup_profile(struct aa_namespace *ns, const char *hname)
{
struct aa_profile *profile;
- read_lock(&ns->lock);
+ rcu_read_lock();
profile = aa_get_profile(__lookup_profile(&ns->base, hname));
- read_unlock(&ns->lock);
+ rcu_read_unlock();
/* the unconfined profile is not in the regular profile list */
if (!profile && strcmp(hname, "unconfined") == 0)
@@ -1091,13 +1102,13 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace)
goto fail;
}
- write_lock(&ns->lock);
+ mutex_lock(&ns->lock);
/* test to see if replacement can be done without failure */
list_for_each_entry(new, &lh, base.list) {
policy = __lookup_replace(ns, new, noreplace, NULL, NULL,
&info);
if (IS_ERR(policy)) {
- write_unlock(&ns->lock);
+ mutex_unlock(&ns->lock);
error = PTR_ERR(policy);
name = new->base.hname;
goto fail;
@@ -1113,6 +1124,7 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace)
list_for_each_entry_safe(new, tmp, &lh, base.list) {
struct aa_profile *old, *rename;
+ /* &lh list is private and not rcu based */
list_del_init(&new->base.list);
policy = __lookup_replace(ns, new, noreplace, &old, &rename,
&info);
@@ -1131,7 +1143,7 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace)
aa_put_profile(old);
aa_put_profile(new);
}
- write_unlock(&ns->lock);
+ mutex_unlock(&ns->lock);
out:
aa_put_namespace(ns);
@@ -1144,6 +1156,7 @@ fail:
error = audit_policy(op, GFP_KERNEL, name, info, error);
list_for_each_entry_safe(new, tmp, &lh, base.list) {
+ /* &lh list is private and not rcu based */
list_del_init(&new->base.list);
aa_put_profile(new);
}
@@ -1196,12 +1209,12 @@ ssize_t aa_remove_profiles(char *fqname, size_t size)
if (!name) {
/* remove namespace - can only happen if fqname[0] == ':' */
- write_lock(&ns->parent->lock);
+ mutex_lock(&ns->parent->lock);
__remove_namespace(ns);
- write_unlock(&ns->parent->lock);
+ mutex_unlock(&ns->parent->lock);
} else {
/* remove profile */
- write_lock(&ns->lock);
+ mutex_lock(&ns->lock);
profile = aa_get_profile(__lookup_profile(&ns->base, name));
if (!profile) {
error = -ENOENT;
@@ -1210,7 +1223,7 @@ ssize_t aa_remove_profiles(char *fqname, size_t size)
}
name = profile->base.hname;
__remove_profile(profile);
- write_unlock(&ns->lock);
+ mutex_unlock(&ns->lock);
}
/* don't fail removal if audit fails */
@@ -1220,7 +1233,7 @@ ssize_t aa_remove_profiles(char *fqname, size_t size)
return size;
fail_ns_lock:
- write_unlock(&ns->lock);
+ mutex_unlock(&ns->lock);
aa_put_namespace(ns);
fail:
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 3d2d0d7..9da2506 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -754,6 +754,11 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns)
return 0;
fail:
+ /*
+ * outside references to the list of profiles are not possible
+ * as they have not been added to the global list yet, so rcu
+ * is not necessary
+ */
list_for_each_entry_safe(profile, tmp, lh, base.list) {
list_del_init(&profile->base.list);
aa_put_profile(profile);
--
1.7.10.4
More information about the AppArmor
mailing list