Holding the preempt_disable is very bad for low latency tasks
such as audio and therefore we need to break out the rule-set dependent
part from this disable. By using a RCU instead of rwlock we
have an efficient locking and less preemption interference.
Selinux uses a lot of read_locks. This patch replaces the rwlock
with RCU that does not hold preempt_disable.
Intel Xeon W3520 2.67 Ghz running FC27 with 4.15.0-rc9git (+measurement)
I get preempt_disable of about 1.2ms in security_compute_av().
With the patch I get 960us as the longest security_compute_av()
without preempt disabeld. There are very much noise in the measurement
but it is not likely a degrade.
And the preempt_disable times is also very dependent on the selinux
rule-set.
In security_get_user_sids() we have two nested for-loops and the
inner part calls sittab_context_to_sid() that calls
sidtab_search_context() that has a for loop() over a while() where
the loops is dependent on the rules.
On the test system the average lookup time is 60us and does
not change with the introduced RCU usage.
The boolean change becomes a lot more heavy with this patch,
but it is a very rare usage in compare with read only operations.
The lock held during a policydb_copy is about 1ms on a XEON.
To use RCU the structure of policydb has to be accesses through a pointer.
We need 5 patches to get there.
[PATCH V3 1/5 selinux-next] selinux: Make allocation atomic in policydb objects functions.
This patch change the allocation for policydb objects. They are in its own patch
to make the complicated part easier to read.
[PATCH V3 2/5 selinux-next] selinux: Introduce selinux_ruleset struct
This makes the access for the rule evaluation going though a single pointer.
[PATCH V3 3/5 selinux-next] selinux: sidtab_clone switch to use rwlock.
We need to make sidtabs copys so this patch change the locks to a rwlock
and create a copy function.
[PATCH V3 4/5 selinux-next] selinux: seqno separation
This patch adds separation of the read and write and uses
the pointer to switch rule set. It uses seqno for error handling
since there are a possibility to have multiple access.
[PATCH V3 5/5 selinux-next] selinux: Switch to rcu read locks for avc_compute
All the preparation is done so this patch do the change of locks to rcu.
History:
V1 rwsem
V2 did not handle all policydb objects, solved with the policydb_copy
did not handle sidtab for booleans, I think this one does however
shutdown is not used but not removed.
We need a copy of sidtabs, so change the generic sidtab_clone
as from a function pointer and let it use a read rwlock while
do the clone.
Signed-off-by: Peter Enderborg <[email protected]>
---
security/selinux/ss/services.c | 20 +-------------------
security/selinux/ss/sidtab.c | 39 ++++++++++++++++++++++++++++++++-------
security/selinux/ss/sidtab.h | 3 ++-
3 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 4f3ce389084c..2be471d72c85 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1891,19 +1891,6 @@ int security_change_sid(struct selinux_state *state,
out_sid, false);
}
-/* Clone the SID into the new SID table. */
-static int clone_sid(u32 sid,
- struct context *context,
- void *arg)
-{
- struct sidtab *s = arg;
-
- if (sid > SECINITSID_NUM)
- return sidtab_insert(s, sid, context);
- else
- return 0;
-}
-
static inline int convert_context_handle_invalid_context(
struct selinux_state *state,
struct context *context)
@@ -2199,10 +2186,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
goto err;
}
- /* Clone the SID table. */
- sidtab_shutdown(old_set->sidtab);
-
- rc = sidtab_map(old_set->sidtab, clone_sid, next_set->sidtab);
+ rc = sidtab_clone(old_set->sidtab, next_set->sidtab);
if (rc)
goto err;
@@ -2926,8 +2910,6 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
goto out;
}
- seqno = ++state->ss->latest_granting;
- state->ss->active_set = next_set;
rc = 0;
out:
if (!rc) {
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index 5be31b7af225..811503cd7c2b 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -27,7 +27,7 @@ int sidtab_init(struct sidtab *s)
s->nel = 0;
s->next_sid = 1;
s->shutdown = 0;
- spin_lock_init(&s->lock);
+ rwlock_init(&s->lock);
return 0;
}
@@ -116,6 +116,31 @@ struct context *sidtab_search_force(struct sidtab *s, u32 sid)
return sidtab_search_core(s, sid, 1);
}
+int sidtab_clone(struct sidtab *s, struct sidtab *d)
+{
+ int i, rc = 0;
+ struct sidtab_node *cur;
+
+ if (!s || !d)
+ goto errout;
+
+ read_lock(&s->lock);
+ for (i = 0; i < SIDTAB_SIZE; i++) {
+ cur = s->htable[i];
+ while (cur) {
+ if (cur->sid > SECINITSID_NUM)
+ rc = sidtab_insert(d, cur->sid, &cur->context);
+ if (rc)
+ goto out;
+ cur = cur->next;
+ }
+ }
+out:
+ read_unlock(&s->lock);
+errout:
+ return rc;
+}
+
int sidtab_map(struct sidtab *s,
int (*apply) (u32 sid,
struct context *context,
@@ -202,7 +227,7 @@ int sidtab_context_to_sid(struct sidtab *s,
if (!sid)
sid = sidtab_search_context(s, context);
if (!sid) {
- spin_lock_irqsave(&s->lock, flags);
+ write_lock_irqsave(&s->lock, flags);
/* Rescan now that we hold the lock. */
sid = sidtab_search_context(s, context);
if (sid)
@@ -221,7 +246,7 @@ int sidtab_context_to_sid(struct sidtab *s,
if (ret)
s->next_sid--;
unlock_out:
- spin_unlock_irqrestore(&s->lock, flags);
+ write_unlock_irqrestore(&s->lock, flags);
}
if (ret)
@@ -287,21 +312,21 @@ void sidtab_set(struct sidtab *dst, struct sidtab *src)
unsigned long flags;
int i;
- spin_lock_irqsave(&src->lock, flags);
+ write_lock_irqsave(&src->lock, flags);
dst->htable = src->htable;
dst->nel = src->nel;
dst->next_sid = src->next_sid;
dst->shutdown = 0;
for (i = 0; i < SIDTAB_CACHE_LEN; i++)
dst->cache[i] = NULL;
- spin_unlock_irqrestore(&src->lock, flags);
+ write_unlock_irqrestore(&src->lock, flags);
}
void sidtab_shutdown(struct sidtab *s)
{
unsigned long flags;
- spin_lock_irqsave(&s->lock, flags);
+ write_lock_irqsave(&s->lock, flags);
s->shutdown = 1;
- spin_unlock_irqrestore(&s->lock, flags);
+ write_unlock_irqrestore(&s->lock, flags);
}
diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
index a1a1d2617b6f..6751f8bcbd66 100644
--- a/security/selinux/ss/sidtab.h
+++ b/security/selinux/ss/sidtab.h
@@ -29,7 +29,7 @@ struct sidtab {
unsigned char shutdown;
#define SIDTAB_CACHE_LEN 3
struct sidtab_node *cache[SIDTAB_CACHE_LEN];
- spinlock_t lock;
+ rwlock_t lock;
};
int sidtab_init(struct sidtab *s);
@@ -51,6 +51,7 @@ void sidtab_hash_eval(struct sidtab *h, char *tag);
void sidtab_destroy(struct sidtab *s);
void sidtab_set(struct sidtab *dst, struct sidtab *src);
void sidtab_shutdown(struct sidtab *s);
+int sidtab_clone(struct sidtab *s, struct sidtab *d);
#endif /* _SS_SIDTAB_H_ */
--
2.15.1
To be able to preempt avc_compute we need preemptible
locks, this patch switch the rwlock reads to rcu_read_lock.
Signed-off-by: Peter Enderborg <[email protected]>
---
security/selinux/ss/services.c | 152 +++++++++++++++++++++--------------------
security/selinux/ss/services.h | 2 +-
2 files changed, 79 insertions(+), 75 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 954ebe490516..a9aa863c47a3 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -84,7 +84,7 @@ static struct selinux_ss selinux_ss;
void selinux_ss_init(struct selinux_ss **ss)
{
- rwlock_init(&selinux_ss.policy_rwlock);
+ spin_lock_init(&selinux_ss.policy_lock);
mutex_init(&selinux_ss.status_lock);
selinux_ss.active_set = kzalloc(sizeof(struct selinux_ruleset),
GFP_KERNEL);
@@ -779,7 +779,7 @@ static int security_compute_validatetrans(struct selinux_state *state,
if (!state->initialized)
return 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -837,7 +837,7 @@ static int security_compute_validatetrans(struct selinux_state *state,
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -879,7 +879,7 @@ int security_bounded_transition(struct selinux_state *state,
if (!state->initialized)
return 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -944,7 +944,7 @@ int security_bounded_transition(struct selinux_state *state,
kfree(old_name);
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -1035,7 +1035,7 @@ void security_compute_xperms_decision(struct selinux_state *state,
memset(xpermd->auditallow->p, 0, sizeof(xpermd->auditallow->p));
memset(xpermd->dontaudit->p, 0, sizeof(xpermd->dontaudit->p));
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
if (!state->initialized)
goto allow;
@@ -1092,7 +1092,7 @@ void security_compute_xperms_decision(struct selinux_state *state,
}
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return;
allow:
memset(xpermd->allowed->p, 0xff, sizeof(xpermd->allowed->p));
@@ -1122,7 +1122,7 @@ void security_compute_av(struct selinux_state *state,
u16 tclass;
struct context *scontext = NULL, *tcontext = NULL;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
avd_init(state, avd);
xperms->len = 0;
if (!state->initialized)
@@ -1160,7 +1160,7 @@ void security_compute_av(struct selinux_state *state,
map_decision(&state->ss->active_set->map, orig_tclass, avd,
policydb->allow_unknown);
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return;
allow:
avd->allowed = 0xffffffff;
@@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state,
struct sidtab *sidtab;
struct context *scontext = NULL, *tcontext = NULL;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
avd_init(state, avd);
if (!state->initialized)
goto allow;
@@ -1212,7 +1212,7 @@ void security_compute_av_user(struct selinux_state *state,
context_struct_compute_av(policydb, scontext, tcontext, tclass, avd,
NULL);
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return;
allow:
avd->allowed = 0xffffffff;
@@ -1319,7 +1319,7 @@ static int security_sid_to_context_core(struct selinux_state *state,
rc = -EINVAL;
goto out;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
if (force)
@@ -1335,7 +1335,7 @@ static int security_sid_to_context_core(struct selinux_state *state,
rc = context_struct_to_string(policydb, context, scontext,
scontext_len);
out_unlock:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
out:
return rc;
@@ -1491,7 +1491,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
if (!str)
goto out;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
rc = string_to_context_struct(policydb, sidtab, scontext2,
@@ -1505,7 +1505,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
rc = sidtab_context_to_sid(sidtab, &context, sid);
context_destroy(&context);
out_unlock:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
out:
kfree(scontext2);
kfree(str);
@@ -1666,7 +1666,7 @@ static int security_compute_sid(struct selinux_state *state,
context_init(&newcontext);
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
if (kern) {
tclass = unmap_class(&state->ss->active_set->map, orig_tclass);
@@ -1806,7 +1806,7 @@ static int security_compute_sid(struct selinux_state *state,
/* Obtain the sid for the context. */
rc = sidtab_context_to_sid(sidtab, &newcontext, out_sid);
out_unlock:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
context_destroy(&newcontext);
out:
return rc;
@@ -2166,14 +2166,14 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
if (rc)
goto pdcerr;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
old_set = state->ss->active_set;
rc = policydb_copy(&old_set->policydb, pdc, &storage, size);
/* save seq */
seqno = state->ss->latest_granting;
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
policydb_flattened_free(storage);
@@ -2232,16 +2232,18 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
goto maperr;
}
+
next_set->map.mapping = newmap.mapping;
next_set->map.size = newmap.size;
/* Install the new policydb and SID table. */
- write_lock_irq(&state->ss->policy_rwlock);
- if (seqno == state->ss->latest_granting) {
+ spin_lock_irq(&state->ss->policy_lock);
+ if (likely(seqno == state->ss->latest_granting)) {
security_load_policycaps(state, &next_set->policydb);
seqno = ++state->ss->latest_granting;
state->ss->active_set = next_set;
- write_unlock_irq(&state->ss->policy_rwlock);
+ spin_unlock_irq(&state->ss->policy_lock);
+ synchronize_rcu();
avc_ss_reset(state->avc, seqno);
selnl_notify_policyload(seqno);
@@ -2260,7 +2262,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
rc = 0;
goto out;
} else {
- write_unlock_irq(&state->ss->policy_rwlock);
+ spin_unlock_irq(&state->ss->policy_lock);
rc = -EAGAIN;
}
maperr:
@@ -2285,9 +2287,9 @@ size_t security_policydb_len(struct selinux_state *state)
struct policydb *p = &state->ss->active_set->policydb;
size_t len;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
len = p->len;
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return len;
}
@@ -2306,7 +2308,7 @@ int security_port_sid(struct selinux_state *state,
struct ocontext *c;
int rc = 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2334,7 +2336,7 @@ int security_port_sid(struct selinux_state *state,
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -2352,7 +2354,7 @@ int security_ib_pkey_sid(struct selinux_state *state,
struct ocontext *c;
int rc = 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2380,7 +2382,7 @@ int security_ib_pkey_sid(struct selinux_state *state,
*out_sid = SECINITSID_UNLABELED;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -2398,7 +2400,7 @@ int security_ib_endport_sid(struct selinux_state *state,
struct ocontext *c;
int rc = 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2427,7 +2429,7 @@ int security_ib_endport_sid(struct selinux_state *state,
*out_sid = SECINITSID_UNLABELED;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -2444,7 +2446,7 @@ int security_netif_sid(struct selinux_state *state,
int rc = 0;
struct ocontext *c;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2474,7 +2476,7 @@ int security_netif_sid(struct selinux_state *state,
*if_sid = SECINITSID_NETIF;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -2509,7 +2511,7 @@ int security_node_sid(struct selinux_state *state,
int rc;
struct ocontext *c;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2567,7 +2569,7 @@ int security_node_sid(struct selinux_state *state,
rc = 0;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -2609,7 +2611,7 @@ int security_get_user_sids(struct selinux_state *state,
if (!state->initialized)
goto out;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2663,7 +2665,7 @@ int security_get_user_sids(struct selinux_state *state,
}
rc = 0;
out_unlock:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
if (rc || !mynel) {
kfree(mysids);
goto out;
@@ -2705,7 +2707,7 @@ int security_get_user_sids(struct selinux_state *state,
* cannot support xattr or use a fixed labeling behavior like
* transition SIDs or task SIDs.
*
- * The caller must acquire the policy_rwlock before calling this function.
+ * The caller must acquire the policy_lock before calling this function.
*/
static inline int __security_genfs_sid(struct selinux_state *state,
const char *fstype,
@@ -2767,7 +2769,7 @@ static inline int __security_genfs_sid(struct selinux_state *state,
* @sclass: file security class
* @sid: SID for path
*
- * Acquire policy_rwlock before calling __security_genfs_sid() and release
+ * Acquire policy_lock before calling __security_genfs_sid() and release
* it afterward.
*/
int security_genfs_sid(struct selinux_state *state,
@@ -2778,9 +2780,9 @@ int security_genfs_sid(struct selinux_state *state,
{
int retval;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
retval = __security_genfs_sid(state, fstype, path, orig_sclass, sid);
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return retval;
}
@@ -2797,7 +2799,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb)
struct superblock_security_struct *sbsec = sb->s_security;
const char *fstype = sb->s_type->name;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
sidtab = state->ss->active_set->sidtab;
@@ -2830,7 +2832,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb)
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -2847,7 +2849,7 @@ int security_get_bools(struct selinux_state *state,
return 0;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
@@ -2880,7 +2882,7 @@ int security_get_bools(struct selinux_state *state,
}
rc = 0;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
err:
if (*names) {
@@ -2914,13 +2916,14 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
goto errout;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
old_set = state->ss->active_set;
seqno = state->ss->latest_granting;
memcpy(next_set, old_set, sizeof(struct selinux_ruleset));
rc = policydb_copy(&old_set->policydb, &next_set->policydb,
&storage, size);
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
+
if (rc)
goto out;
@@ -2956,12 +2959,13 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
rc = 0;
out:
if (!rc) {
- write_lock_irq(&state->ss->policy_rwlock);
+ spin_lock_irq(&state->ss->policy_lock);
if (seqno == state->ss->latest_granting) {
seqno = ++state->ss->latest_granting;
state->ss->active_set = next_set;
rc = 0;
- write_unlock_irq(&state->ss->policy_rwlock);
+ spin_unlock_irq(&state->ss->policy_lock);
+ synchronize_rcu();
avc_ss_reset(state->avc, seqno);
selnl_notify_policyload(seqno);
selinux_status_update_policyload(state, seqno);
@@ -2970,7 +2974,7 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
kfree(old_set);
} else {
rc = -ENOMEM;
- write_unlock_irq(&state->ss->policy_rwlock);
+ spin_unlock_irq(&state->ss->policy_lock);
printk(KERN_ERR "SELinux: %s failed in seqno %d\n",
__func__, rc);
policydb_destroy(&next_set->policydb);
@@ -2994,7 +2998,7 @@ int security_get_bool_value(struct selinux_state *state,
int rc;
int len;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
policydb = &state->ss->active_set->policydb;
@@ -3005,7 +3009,7 @@ int security_get_bool_value(struct selinux_state *state,
rc = policydb->bool_val_to_struct[index]->state;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -3065,7 +3069,7 @@ int security_sid_mls_copy(struct selinux_state *state,
context_init(&newcon);
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = -EINVAL;
context1 = sidtab_search(sidtab, sid);
@@ -3108,7 +3112,7 @@ int security_sid_mls_copy(struct selinux_state *state,
rc = sidtab_context_to_sid(sidtab, &newcon, new_sid);
out_unlock:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
context_destroy(&newcon);
out:
return rc;
@@ -3170,7 +3174,7 @@ int security_net_peersid_resolve(struct selinux_state *state,
if (!policydb->mls_enabled)
return 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = -EINVAL;
nlbl_ctx = sidtab_search(sidtab, nlbl_sid);
@@ -3197,7 +3201,7 @@ int security_net_peersid_resolve(struct selinux_state *state,
* expressive */
*peer_sid = xfrm_sid;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -3226,7 +3230,7 @@ int security_get_classes(struct selinux_state *state,
return 0;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = -ENOMEM;
*nclasses = policydb->p_classes.nprim;
@@ -3244,7 +3248,7 @@ int security_get_classes(struct selinux_state *state,
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -3268,7 +3272,7 @@ int security_get_permissions(struct selinux_state *state,
int rc, i;
struct class_datum *match;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = -EINVAL;
match = hashtab_search(policydb->p_classes.table, class);
@@ -3297,11 +3301,11 @@ int security_get_permissions(struct selinux_state *state,
goto err;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
err:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
for (i = 0; i < *nperms; i++)
kfree((*perms)[i]);
kfree(*perms);
@@ -3334,9 +3338,9 @@ int security_policycap_supported(struct selinux_state *state,
struct policydb *policydb = &state->ss->active_set->policydb;
int rc;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = ebitmap_get_bit(&policydb->policycaps, req_cap);
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -3402,7 +3406,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
context_init(&tmprule->au_ctxt);
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
tmprule->au_seqno = state->ss->latest_granting;
@@ -3443,7 +3447,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
}
rc = 0;
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
if (rc) {
selinux_audit_rule_free(tmprule);
@@ -3494,7 +3498,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule,
return -ENOENT;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
if (rule->au_seqno < state->ss->latest_granting) {
match = -ESTALE;
@@ -3585,7 +3589,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule,
}
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return match;
}
@@ -3674,7 +3678,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
return 0;
}
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
if (secattr->flags & NETLBL_SECATTR_CACHE)
*sid = *(u32 *)secattr->cache->data;
@@ -3710,12 +3714,12 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
} else
*sid = SECSID_NULL;
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return 0;
out_free:
ebitmap_destroy(&ctx_new.range.level[0].cat);
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
@@ -3739,7 +3743,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
if (!state->initialized)
return 0;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = -ENOENT;
ctx = sidtab_search(state->ss->active_set->sidtab, sid);
@@ -3757,7 +3761,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
mls_export_netlbl_lvl(policydb, ctx, secattr);
rc = mls_export_netlbl_cat(policydb, ctx, secattr);
out:
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
return rc;
}
#endif /* CONFIG_NETLABEL */
@@ -3787,9 +3791,9 @@ int security_read_policy(struct selinux_state *state,
fp.data = *data;
fp.len = *len;
- read_lock(&state->ss->policy_rwlock);
+ rcu_read_lock();
rc = policydb_write(policydb, &fp);
- read_unlock(&state->ss->policy_rwlock);
+ rcu_read_unlock();
if (rc)
return rc;
diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
index 9219649c70ed..84cbcf226551 100644
--- a/security/selinux/ss/services.h
+++ b/security/selinux/ss/services.h
@@ -33,7 +33,7 @@ struct selinux_ruleset {
};
struct selinux_ss {
struct selinux_ruleset *active_set; /* rcu pointer */
- rwlock_t policy_rwlock;
+ spinlock_t policy_lock;
u32 latest_granting;
struct page *status_page;
struct mutex status_lock;
--
2.15.1
From: peter <[email protected]>
As preparation for RCU the allocation need to be atomic,
there is a lot of them so they do in this patch.
Signed-off-by: Peter Enderborg <[email protected]>
---
security/selinux/ss/avtab.c | 8 +--
security/selinux/ss/conditional.c | 14 ++---
security/selinux/ss/ebitmap.c | 3 +-
security/selinux/ss/hashtab.c | 6 +--
security/selinux/ss/policydb.c | 104 +++++++++++++++++++-------------------
5 files changed, 69 insertions(+), 66 deletions(-)
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index a2c9148b0662..1114a308aa94 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -72,13 +72,13 @@ avtab_insert_node(struct avtab *h, int hvalue,
{
struct avtab_node *newnode;
struct avtab_extended_perms *xperms;
- newnode = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
+ newnode = kmem_cache_zalloc(avtab_node_cachep, GFP_ATOMIC);
if (newnode == NULL)
return NULL;
newnode->key = *key;
if (key->specified & AVTAB_XPERMS) {
- xperms = kmem_cache_zalloc(avtab_xperms_cachep, GFP_KERNEL);
+ xperms = kmem_cache_zalloc(avtab_xperms_cachep, GFP_ATOMIC);
if (xperms == NULL) {
kmem_cache_free(avtab_node_cachep, newnode);
return NULL;
@@ -95,7 +95,7 @@ avtab_insert_node(struct avtab *h, int hvalue,
} else {
newnode->next = flex_array_get_ptr(h->htable, hvalue);
if (flex_array_put_ptr(h->htable, hvalue, newnode,
- GFP_KERNEL|__GFP_ZERO)) {
+ GFP_ATOMIC|__GFP_ZERO)) {
kmem_cache_free(avtab_node_cachep, newnode);
return NULL;
}
@@ -330,7 +330,7 @@ int avtab_alloc(struct avtab *h, u32 nrules)
mask = nslot - 1;
h->htable = flex_array_alloc(sizeof(struct avtab_node *), nslot,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_ATOMIC | __GFP_ZERO);
if (!h->htable)
return -ENOMEM;
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index c91543a617ac..a09c8a8e9472 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -178,7 +178,7 @@ int cond_init_bool_indexes(struct policydb *p)
kfree(p->bool_val_to_struct);
p->bool_val_to_struct = kmalloc_array(p->p_bools.nprim,
sizeof(*p->bool_val_to_struct),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!p->bool_val_to_struct)
return -ENOMEM;
return 0;
@@ -205,7 +205,7 @@ int cond_index_bool(void *key, void *datum, void *datap)
fa = p->sym_val_to_name[SYM_BOOLS];
if (flex_array_put_ptr(fa, booldatum->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
p->bool_val_to_struct[booldatum->value - 1] = booldatum;
@@ -227,7 +227,7 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
u32 len;
int rc;
- booldatum = kzalloc(sizeof(*booldatum), GFP_KERNEL);
+ booldatum = kzalloc(sizeof(*booldatum), GFP_ATOMIC);
if (!booldatum)
return -ENOMEM;
@@ -247,7 +247,7 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
goto err;
rc = -ENOMEM;
- key = kmalloc(len + 1, GFP_KERNEL);
+ key = kmalloc(len + 1, GFP_ATOMIC);
if (!key)
goto err;
rc = next_entry(key, fp, len);
@@ -332,7 +332,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
goto err;
}
- list = kzalloc(sizeof(*list), GFP_KERNEL);
+ list = kzalloc(sizeof(*list), GFP_ATOMIC);
if (!list) {
rc = -ENOMEM;
goto err;
@@ -420,7 +420,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
goto err;
rc = -ENOMEM;
- expr = kzalloc(sizeof(*expr), GFP_KERNEL);
+ expr = kzalloc(sizeof(*expr), GFP_ATOMIC);
if (!expr)
goto err;
@@ -471,7 +471,7 @@ int cond_read_list(struct policydb *p, void *fp)
for (i = 0; i < len; i++) {
rc = -ENOMEM;
- node = kzalloc(sizeof(*node), GFP_KERNEL);
+ node = kzalloc(sizeof(*node), GFP_ATOMIC);
if (!node)
goto err;
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 5ae8c61b75bf..a49fabe6f744 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -403,7 +403,8 @@ int ebitmap_read(struct ebitmap *e, void *fp)
if (!n || startbit >= n->startbit + EBITMAP_SIZE) {
struct ebitmap_node *tmp;
- tmp = kmem_cache_zalloc(ebitmap_node_cachep, GFP_KERNEL);
+ tmp = kmem_cache_zalloc(ebitmap_node_cachep,
+ GFP_ATOMIC);
if (!tmp) {
printk(KERN_ERR
"SELinux: ebitmap: out of memory\n");
diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index ebfdaa31ee32..0944b1f8060e 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -19,7 +19,7 @@ struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void *
struct hashtab *p;
u32 i;
- p = kzalloc(sizeof(*p), GFP_KERNEL);
+ p = kzalloc(sizeof(*p), GFP_ATOMIC);
if (!p)
return p;
@@ -27,7 +27,7 @@ struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void *
p->nel = 0;
p->hash_value = hash_value;
p->keycmp = keycmp;
- p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_KERNEL);
+ p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_ATOMIC);
if (!p->htable) {
kfree(p);
return NULL;
@@ -60,7 +60,7 @@ int hashtab_insert(struct hashtab *h, void *key, void *datum)
if (cur && (h->keycmp(h, key, cur->key) == 0))
return -EEXIST;
- newnode = kmem_cache_zalloc(hashtab_node_cachep, GFP_KERNEL);
+ newnode = kmem_cache_zalloc(hashtab_node_cachep, GFP_ATOMIC);
if (!newnode)
return -ENOMEM;
newnode->key = key;
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 6e8c8056d7ad..2a0e21d8c275 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -188,7 +188,7 @@ static int roles_init(struct policydb *p)
int rc;
struct role_datum *role;
- role = kzalloc(sizeof(*role), GFP_KERNEL);
+ role = kzalloc(sizeof(*role), GFP_ATOMIC);
if (!role)
return -ENOMEM;
@@ -198,7 +198,7 @@ static int roles_init(struct policydb *p)
goto out;
rc = -ENOMEM;
- key = kstrdup(OBJECT_R, GFP_KERNEL);
+ key = kstrdup(OBJECT_R, GFP_ATOMIC);
if (!key)
goto out;
@@ -350,7 +350,7 @@ static int common_index(void *key, void *datum, void *datap)
fa = p->sym_val_to_name[SYM_COMMONS];
if (flex_array_put_ptr(fa, comdatum->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
return 0;
}
@@ -367,7 +367,7 @@ static int class_index(void *key, void *datum, void *datap)
return -EINVAL;
fa = p->sym_val_to_name[SYM_CLASSES];
if (flex_array_put_ptr(fa, cladatum->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
p->class_val_to_struct[cladatum->value - 1] = cladatum;
return 0;
@@ -388,7 +388,7 @@ static int role_index(void *key, void *datum, void *datap)
fa = p->sym_val_to_name[SYM_ROLES];
if (flex_array_put_ptr(fa, role->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
p->role_val_to_struct[role->value - 1] = role;
return 0;
@@ -410,12 +410,12 @@ static int type_index(void *key, void *datum, void *datap)
return -EINVAL;
fa = p->sym_val_to_name[SYM_TYPES];
if (flex_array_put_ptr(fa, typdatum->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
fa = p->type_val_to_struct_array;
if (flex_array_put_ptr(fa, typdatum->value - 1, typdatum,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
}
@@ -437,7 +437,7 @@ static int user_index(void *key, void *datum, void *datap)
fa = p->sym_val_to_name[SYM_USERS];
if (flex_array_put_ptr(fa, usrdatum->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
p->user_val_to_struct[usrdatum->value - 1] = usrdatum;
return 0;
@@ -458,7 +458,7 @@ static int sens_index(void *key, void *datum, void *datap)
return -EINVAL;
fa = p->sym_val_to_name[SYM_LEVELS];
if (flex_array_put_ptr(fa, levdatum->level->sens - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
}
@@ -479,7 +479,7 @@ static int cat_index(void *key, void *datum, void *datap)
return -EINVAL;
fa = p->sym_val_to_name[SYM_CATS];
if (flex_array_put_ptr(fa, catdatum->value - 1, key,
- GFP_KERNEL | __GFP_ZERO))
+ GFP_ATOMIC | __GFP_ZERO))
BUG();
}
@@ -550,31 +550,31 @@ static int policydb_index(struct policydb *p)
p->class_val_to_struct = kcalloc(p->p_classes.nprim,
sizeof(*p->class_val_to_struct),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!p->class_val_to_struct)
return -ENOMEM;
p->role_val_to_struct = kcalloc(p->p_roles.nprim,
sizeof(*p->role_val_to_struct),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!p->role_val_to_struct)
return -ENOMEM;
p->user_val_to_struct = kcalloc(p->p_users.nprim,
sizeof(*p->user_val_to_struct),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!p->user_val_to_struct)
return -ENOMEM;
/* Yes, I want the sizeof the pointer, not the structure */
p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
p->p_types.nprim,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_ATOMIC | __GFP_ZERO);
if (!p->type_val_to_struct_array)
return -ENOMEM;
rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
- p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
+ p->p_types.nprim, GFP_ATOMIC | __GFP_ZERO);
if (rc)
goto out;
@@ -585,13 +585,14 @@ static int policydb_index(struct policydb *p)
for (i = 0; i < SYM_NUM; i++) {
p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
p->symtab[i].nprim,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_ATOMIC |
+ __GFP_ZERO);
if (!p->sym_val_to_name[i])
return -ENOMEM;
rc = flex_array_prealloc(p->sym_val_to_name[i],
0, p->symtab[i].nprim,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_ATOMIC | __GFP_ZERO);
if (rc)
goto out;
@@ -1122,7 +1123,7 @@ static int perm_read(struct policydb *p, struct hashtab *h, void *fp)
__le32 buf[2];
u32 len;
- perdatum = kzalloc(sizeof(*perdatum), GFP_KERNEL);
+ perdatum = kzalloc(sizeof(*perdatum), GFP_ATOMIC);
if (!perdatum)
return -ENOMEM;
@@ -1133,7 +1134,7 @@ static int perm_read(struct policydb *p, struct hashtab *h, void *fp)
len = le32_to_cpu(buf[0]);
perdatum->value = le32_to_cpu(buf[1]);
- rc = str_read(&key, GFP_KERNEL, fp, len);
+ rc = str_read(&key, GFP_ATOMIC, fp, len);
if (rc)
goto bad;
@@ -1155,7 +1156,7 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp)
u32 len, nel;
int i, rc;
- comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL);
+ comdatum = kzalloc(sizeof(*comdatum), GFP_ATOMIC);
if (!comdatum)
return -ENOMEM;
@@ -1172,7 +1173,7 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp)
comdatum->permissions.nprim = le32_to_cpu(buf[2]);
nel = le32_to_cpu(buf[3]);
- rc = str_read(&key, GFP_KERNEL, fp, len);
+ rc = str_read(&key, GFP_ATOMIC, fp, len);
if (rc)
goto bad;
@@ -1228,7 +1229,7 @@ static int read_cons_helper(struct policydb *p,
lc = NULL;
for (i = 0; i < ncons; i++) {
- c = kzalloc(sizeof(*c), GFP_KERNEL);
+ c = kzalloc(sizeof(*c), GFP_ATOMIC);
if (!c)
return -ENOMEM;
@@ -1245,7 +1246,7 @@ static int read_cons_helper(struct policydb *p,
le = NULL;
depth = -1;
for (j = 0; j < nexpr; j++) {
- e = kzalloc(sizeof(*e), GFP_KERNEL);
+ e = kzalloc(sizeof(*e), GFP_ATOMIC);
if (!e)
return -ENOMEM;
@@ -1290,7 +1291,7 @@ static int read_cons_helper(struct policydb *p,
POLICYDB_VERSION_CONSTRAINT_NAMES) {
e->type_names = kzalloc(sizeof
(*e->type_names),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!e->type_names)
return -ENOMEM;
type_set_init(e->type_names);
@@ -1320,7 +1321,7 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp)
u32 len, len2, ncons, nel;
int i, rc;
- cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
+ cladatum = kzalloc(sizeof(*cladatum), GFP_ATOMIC);
if (!cladatum)
return -ENOMEM;
@@ -1340,12 +1341,12 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp)
ncons = le32_to_cpu(buf[5]);
- rc = str_read(&key, GFP_KERNEL, fp, len);
+ rc = str_read(&key, GFP_ATOMIC, fp, len);
if (rc)
goto bad;
if (len2) {
- rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
+ rc = str_read(&cladatum->comkey, GFP_ATOMIC, fp, len2);
if (rc)
goto bad;
@@ -1413,7 +1414,7 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp)
__le32 buf[3];
u32 len;
- role = kzalloc(sizeof(*role), GFP_KERNEL);
+ role = kzalloc(sizeof(*role), GFP_ATOMIC);
if (!role)
return -ENOMEM;
@@ -1429,7 +1430,7 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp)
if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
role->bounds = le32_to_cpu(buf[2]);
- rc = str_read(&key, GFP_KERNEL, fp, len);
+ rc = str_read(&key, GFP_ATOMIC, fp, len);
if (rc)
goto bad;
@@ -1469,7 +1470,7 @@ static int type_read(struct policydb *p, struct hashtab *h, void *fp)
__le32 buf[4];
u32 len;
- typdatum = kzalloc(sizeof(*typdatum), GFP_KERNEL);
+ typdatum = kzalloc(sizeof(*typdatum), GFP_ATOMIC);
if (!typdatum)
return -ENOMEM;
@@ -1495,7 +1496,7 @@ static int type_read(struct policydb *p, struct hashtab *h, void *fp)
typdatum->primary = le32_to_cpu(buf[2]);
}
- rc = str_read(&key, GFP_KERNEL, fp, len);
+ rc = str_read(&key, GFP_ATOMIC, fp, len);
if (rc)
goto bad;
@@ -1543,7 +1544,7 @@ static int user_read(struct policydb *p, struct hashtab *h, void *fp)
__le32 buf[3];
u32 len;
- usrdatum = kzalloc(sizeof(*usrdatum), GFP_KERNEL);
+ usrdatum = kzalloc(sizeof(*usrdatum), GFP_ATOMIC);
if (!usrdatum)
return -ENOMEM;
@@ -1559,7 +1560,7 @@ static int user_read(struct policydb *p, struct hashtab *h, void *fp)
if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
usrdatum->bounds = le32_to_cpu(buf[2]);
- rc = str_read(&key, GFP_KERNEL, fp, len);
+ rc = str_read(&key, GFP_ATOMIC, fp, len);
if (rc)
goto bad;
@@ -1853,7 +1854,7 @@ static int range_read(struct policydb *p, void *fp)
nel = le32_to_cpu(buf[0]);
for (i = 0; i < nel; i++) {
rc = -ENOMEM;
- rt = kzalloc(sizeof(*rt), GFP_KERNEL);
+ rt = kzalloc(sizeof(*rt), GFP_ATOMIC);
if (!rt)
goto out;
@@ -1878,7 +1879,7 @@ static int range_read(struct policydb *p, void *fp)
goto out;
rc = -ENOMEM;
- r = kzalloc(sizeof(*r), GFP_KERNEL);
+ r = kzalloc(sizeof(*r), GFP_ATOMIC);
if (!r)
goto out;
@@ -1929,12 +1930,12 @@ static int filename_trans_read(struct policydb *p, void *fp)
name = NULL;
rc = -ENOMEM;
- ft = kzalloc(sizeof(*ft), GFP_KERNEL);
+ ft = kzalloc(sizeof(*ft), GFP_ATOMIC);
if (!ft)
goto out;
rc = -ENOMEM;
- otype = kmalloc(sizeof(*otype), GFP_KERNEL);
+ otype = kmalloc(sizeof(*otype), GFP_ATOMIC);
if (!otype)
goto out;
@@ -1945,7 +1946,7 @@ static int filename_trans_read(struct policydb *p, void *fp)
len = le32_to_cpu(buf[0]);
/* path component string */
- rc = str_read(&name, GFP_KERNEL, fp, len);
+ rc = str_read(&name, GFP_ATOMIC, fp, len);
if (rc)
goto out;
@@ -2011,11 +2012,11 @@ static int genfs_read(struct policydb *p, void *fp)
len = le32_to_cpu(buf[0]);
rc = -ENOMEM;
- newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL);
+ newgenfs = kzalloc(sizeof(*newgenfs), GFP_ATOMIC);
if (!newgenfs)
goto out;
- rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
+ rc = str_read(&newgenfs->fstype, GFP_ATOMIC, fp, len);
if (rc)
goto out;
@@ -2050,11 +2051,11 @@ static int genfs_read(struct policydb *p, void *fp)
len = le32_to_cpu(buf[0]);
rc = -ENOMEM;
- newc = kzalloc(sizeof(*newc), GFP_KERNEL);
+ newc = kzalloc(sizeof(*newc), GFP_ATOMIC);
if (!newc)
goto out;
- rc = str_read(&newc->u.name, GFP_KERNEL, fp, len);
+ rc = str_read(&newc->u.name, GFP_ATOMIC, fp, len);
if (rc)
goto out;
@@ -2120,7 +2121,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
l = NULL;
for (j = 0; j < nel; j++) {
rc = -ENOMEM;
- c = kzalloc(sizeof(*c), GFP_KERNEL);
+ c = kzalloc(sizeof(*c), GFP_ATOMIC);
if (!c)
goto out;
if (l)
@@ -2147,7 +2148,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
goto out;
len = le32_to_cpu(buf[0]);
- rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+ rc = str_read(&c->u.name, GFP_ATOMIC, fp, len);
if (rc)
goto out;
@@ -2193,7 +2194,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
goto out;
len = le32_to_cpu(buf[1]);
- rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+ rc = str_read(&c->u.name, GFP_ATOMIC, fp, len);
if (rc)
goto out;
@@ -2244,7 +2245,8 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
goto out;
len = le32_to_cpu(buf[0]);
- rc = str_read(&c->u.ibendport.dev_name, GFP_KERNEL, fp, len);
+ rc = str_read(&c->u.ibendport.dev_name,
+ GFP_ATOMIC, fp, len);
if (rc)
goto out;
@@ -2311,7 +2313,7 @@ int policydb_read(struct policydb *p, void *fp)
}
rc = -ENOMEM;
- policydb_str = kmalloc(len + 1, GFP_KERNEL);
+ policydb_str = kmalloc(len + 1, GFP_ATOMIC);
if (!policydb_str) {
printk(KERN_ERR "SELinux: unable to allocate memory for policydb "
"string of length %d\n", len);
@@ -2433,7 +2435,7 @@ int policydb_read(struct policydb *p, void *fp)
ltr = NULL;
for (i = 0; i < nel; i++) {
rc = -ENOMEM;
- tr = kzalloc(sizeof(*tr), GFP_KERNEL);
+ tr = kzalloc(sizeof(*tr), GFP_ATOMIC);
if (!tr)
goto bad;
if (ltr)
@@ -2472,7 +2474,7 @@ int policydb_read(struct policydb *p, void *fp)
lra = NULL;
for (i = 0; i < nel; i++) {
rc = -ENOMEM;
- ra = kzalloc(sizeof(*ra), GFP_KERNEL);
+ ra = kzalloc(sizeof(*ra), GFP_ATOMIC);
if (!ra)
goto bad;
if (lra)
@@ -2521,13 +2523,13 @@ int policydb_read(struct policydb *p, void *fp)
rc = -ENOMEM;
p->type_attr_map_array = flex_array_alloc(sizeof(struct ebitmap),
p->p_types.nprim,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_ATOMIC | __GFP_ZERO);
if (!p->type_attr_map_array)
goto bad;
/* preallocate so we don't have to worry about the put ever failing */
rc = flex_array_prealloc(p->type_attr_map_array, 0, p->p_types.nprim,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_ATOMIC | __GFP_ZERO);
if (rc)
goto bad;
--
2.15.1
This patch separtate the locks for read and write, and
to be sure that they are using the same structure the
seqno is used. If the seqno is changed from the read to
write section the function reportes an eagain error.
Signed-off-by: Peter Enderborg <[email protected]>
---
security/selinux/ss/services.c | 143 ++++++++++++++++++++++++++++-------------
1 file changed, 98 insertions(+), 45 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2be471d72c85..954ebe490516 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2104,6 +2104,9 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
u32 seqno;
int rc = 0;
struct selinux_ruleset *next_set, *old_set;
+ size_t size;
+ void *storage;
+ struct policydb *pdc;
struct policy_file file = { data, len }, *fp = &file;
next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
@@ -2111,14 +2114,15 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
rc = -ENOMEM;
goto out;
}
+
next_set->sidtab = kzalloc(sizeof(struct sidtab), GFP_KERNEL);
if (!next_set->sidtab) {
rc = -ENOMEM;
- kfree(next_set);
- goto out;
+ goto nexterr;
}
if (!state->initialized) {
+ /* sidtab exist before inititalisation */
old_set = state->ss->active_set;
rc = policydb_read(&next_set->policydb, fp);
if (rc)
@@ -2152,57 +2156,80 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
kfree(old_set);
goto out;
}
+
+ pdc = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
+ if (!pdc)
+ goto allocerr;
+
+ rc = policydb_flattened_alloc(&state->ss->active_set->policydb,
+ &storage, &size);
+ if (rc)
+ goto pdcerr;
+
+ read_lock(&state->ss->policy_rwlock);
old_set = state->ss->active_set;
+ rc = policydb_copy(&old_set->policydb, pdc, &storage, size);
+
+ /* save seq */
+ seqno = state->ss->latest_granting;
+
+ read_unlock(&state->ss->policy_rwlock);
+
+ policydb_flattened_free(storage);
+
+ if (rc)
+ goto cpyerr;
+
#if 0
sidtab_hash_eval(sidtab, "sids");
#endif
-
rc = policydb_read(&next_set->policydb, fp);
if (rc)
- goto out;
+ goto cpyerr;
next_set->policydb.len = len;
/* If switching between different policy types, log MLS status */
- if (old_set->policydb.mls_enabled && !next_set->policydb.mls_enabled)
+ if (pdc->mls_enabled && !next_set->policydb.mls_enabled)
printk(KERN_INFO "SELinux: Disabling MLS support...\n");
- else if (!old_set->policydb.mls_enabled
+ else if (!pdc->mls_enabled
&& next_set->policydb.mls_enabled)
printk(KERN_INFO "SELinux: Enabling MLS support...\n");
+
rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
if (rc) {
printk(KERN_ERR "SELinux: unable to load the initial SIDs\n");
- policydb_destroy(&next_set->policydb);
- goto out;
+ goto cpyerr;
}
rc = selinux_set_mapping(&next_set->policydb, secclass_map, &newmap);
if (rc)
- goto err;
+ goto loaderr;
rc = security_preserve_bools(state, &next_set->policydb);
if (rc) {
printk(KERN_ERR "SELinux: unable to preserve booleans\n");
- goto err;
+ goto maperr;
}
rc = sidtab_clone(old_set->sidtab, next_set->sidtab);
if (rc)
- goto err;
+ goto maperr;
/*
* Convert the internal representations of contexts
* in the new SID table.
*/
args.state = state;
- args.oldp = &old_set->policydb;
+ args.oldp = pdc;
args.newp = &next_set->policydb;
+
rc = sidtab_map(next_set->sidtab, convert_context, &args);
if (rc) {
printk(KERN_ERR "SELinux: unable to convert the internal"
" representation of contexts in the new SID"
" table\n");
- goto err;
+ goto maperr;
}
next_set->map.mapping = newmap.mapping;
@@ -2210,30 +2237,44 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
/* Install the new policydb and SID table. */
write_lock_irq(&state->ss->policy_rwlock);
- security_load_policycaps(state, &next_set->policydb);
- seqno = ++state->ss->latest_granting;
- state->ss->active_set = next_set;
- write_unlock_irq(&state->ss->policy_rwlock);
-
- avc_ss_reset(state->avc, seqno);
- selnl_notify_policyload(seqno);
- selinux_status_update_policyload(state, seqno);
- selinux_netlbl_cache_invalidate();
- selinux_xfrm_notify_policyload();
-
- /* Free the old policydb and SID table. */
- policydb_destroy(&old_set->policydb);
- sidtab_destroy(old_set->sidtab);
- kfree(old_set->sidtab);
- kfree(old_set->map.mapping);
- kfree(old_set);
- rc = 0;
- goto out;
+ if (seqno == state->ss->latest_granting) {
+ security_load_policycaps(state, &next_set->policydb);
+ seqno = ++state->ss->latest_granting;
+ state->ss->active_set = next_set;
+ write_unlock_irq(&state->ss->policy_rwlock);
-err:
+ avc_ss_reset(state->avc, seqno);
+ selnl_notify_policyload(seqno);
+ selinux_status_update_policyload(state, seqno);
+ selinux_netlbl_cache_invalidate();
+ selinux_xfrm_notify_policyload();
+
+ /* Free the old policydb and SID table. */
+ policydb_destroy(pdc);
+ kfree(pdc);
+ sidtab_destroy(old_set->sidtab);
+ policydb_destroy(&old_set->policydb);
+ kfree(old_set->sidtab);
+ kfree(old_set->map.mapping);
+ kfree(old_set);
+ rc = 0;
+ goto out;
+ } else {
+ write_unlock_irq(&state->ss->policy_rwlock);
+ rc = -EAGAIN;
+ }
+maperr:
kfree(newmap.mapping);
+loaderr:
sidtab_destroy(next_set->sidtab);
+cpyerr:
policydb_destroy(&next_set->policydb);
+ policydb_destroy(pdc);
+pdcerr:
+ kfree(pdc);
+allocerr:
+ kfree(next_set->sidtab);
+nexterr:
kfree(next_set);
out:
return rc;
@@ -2873,11 +2914,13 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
goto errout;
}
- write_lock_irq(&state->ss->policy_rwlock);
+ read_lock(&state->ss->policy_rwlock);
old_set = state->ss->active_set;
+ seqno = state->ss->latest_granting;
memcpy(next_set, old_set, sizeof(struct selinux_ruleset));
rc = policydb_copy(&old_set->policydb, &next_set->policydb,
&storage, size);
+ read_unlock(&state->ss->policy_rwlock);
if (rc)
goto out;
@@ -2913,19 +2956,29 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
rc = 0;
out:
if (!rc) {
- seqno = ++state->ss->latest_granting;
- state->ss->active_set = next_set;
- rc = 0;
- write_unlock_irq(&state->ss->policy_rwlock);
- avc_ss_reset(state->avc, seqno);
- selnl_notify_policyload(seqno);
- selinux_status_update_policyload(state, seqno);
- selinux_xfrm_notify_policyload();
- policydb_destroy(&old_set->policydb);
- kfree(old_set);
+ write_lock_irq(&state->ss->policy_rwlock);
+ if (seqno == state->ss->latest_granting) {
+ seqno = ++state->ss->latest_granting;
+ state->ss->active_set = next_set;
+ rc = 0;
+ write_unlock_irq(&state->ss->policy_rwlock);
+ avc_ss_reset(state->avc, seqno);
+ selnl_notify_policyload(seqno);
+ selinux_status_update_policyload(state, seqno);
+ selinux_xfrm_notify_policyload();
+ policydb_destroy(&old_set->policydb);
+ kfree(old_set);
+ } else {
+ rc = -ENOMEM;
+ write_unlock_irq(&state->ss->policy_rwlock);
+ printk(KERN_ERR "SELinux: %s failed in seqno %d\n",
+ __func__, rc);
+ policydb_destroy(&next_set->policydb);
+ kfree(next_set);
+ }
} else {
printk(KERN_ERR "SELinux: %s failed %d\n", __func__, rc);
- write_unlock_irq(&state->ss->policy_rwlock);
+ policydb_destroy(&next_set->policydb);
kfree(next_set);
}
policydb_flattened_free(storage);
--
2.15.1
This is a preparation for moving locking to rcu type.
We move policydb, sidtab and map to this structure which
is dynamic allocated. To help out the handlig a policydb_copy
are added. It is intended to be used in atomic context within
a rcu lock, so there are help functions that do vmalloc
allocation that are intended to be on the outside of the lock.
hastab_insert had a cond_sched call that is removed. When switched
to rcu lock the lock can be preempted.
Signed-off-by: Peter Enderborg <[email protected]>
---
security/selinux/ss/hashtab.c | 1 -
security/selinux/ss/policydb.c | 48 +++++++
security/selinux/ss/policydb.h | 6 +-
security/selinux/ss/services.c | 292 +++++++++++++++++++++++------------------
security/selinux/ss/services.h | 12 +-
5 files changed, 226 insertions(+), 133 deletions(-)
diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index 0944b1f8060e..967b6e3d25c6 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -44,7 +44,6 @@ int hashtab_insert(struct hashtab *h, void *key, void *datum)
u32 hvalue;
struct hashtab_node *prev, *cur, *newnode;
- cond_resched();
if (!h || h->nel == HASHTAB_MAX_NODES)
return -EINVAL;
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 2a0e21d8c275..93d134d057a7 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -3535,3 +3535,51 @@ int policydb_write(struct policydb *p, void *fp)
return 0;
}
+
+int policydb_flattened_alloc(struct policydb *db, void **tmpbuf, size_t *size)
+{
+ int rc = 0;
+
+ *size = db->len;
+ *tmpbuf = vmalloc(*size);
+
+ if (!*tmpbuf) {
+ rc = -ENOMEM;
+ printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
+ }
+ return rc;
+}
+
+int policydb_flattened_free(void *tmpbuf)
+{
+ vfree(tmpbuf);
+ return 0;
+}
+
+int policydb_copy(struct policydb *olddb, struct policydb *newdb,
+ void **tmpstorage, size_t size)
+{
+ struct policy_file fp;
+ void *data = *tmpstorage;
+ int rc;
+
+ if (size != olddb->len) {
+ rc = -EAGAIN;
+ goto out;
+ }
+ fp.data = data;
+ fp.len = size;
+ rc = policydb_write(olddb, &fp);
+ if (rc)
+ goto out;
+
+ fp.len = size;
+ fp.data = data;
+ rc = policydb_read(newdb, &fp);
+ if (rc)
+ goto out;
+
+ newdb->len = size;
+out:
+ return rc;
+}
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 215f8f30ac5a..3e2f86b5b674 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -320,7 +320,11 @@ extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
extern int policydb_read(struct policydb *p, void *fp);
extern int policydb_write(struct policydb *p, void *fp);
-
+extern int policydb_copy(struct policydb *olddb, struct policydb *newdb,
+ void **tmpstorage, size_t size);
+extern int policydb_flattened_alloc(struct policydb *db,
+ void **tmpbuf, size_t *size);
+extern int policydb_flattened_free(void *tmpbuf);
#define PERM_SYMTAB_SIZE 32
#define POLICYDB_CONFIG_MLS 1
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 8057e19dc15f..4f3ce389084c 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -86,6 +86,10 @@ void selinux_ss_init(struct selinux_ss **ss)
{
rwlock_init(&selinux_ss.policy_rwlock);
mutex_init(&selinux_ss.status_lock);
+ selinux_ss.active_set = kzalloc(sizeof(struct selinux_ruleset),
+ GFP_KERNEL);
+ selinux_ss.active_set->sidtab = kzalloc(sizeof(struct sidtab),
+ GFP_KERNEL);
*ss = &selinux_ss;
}
@@ -249,7 +253,7 @@ static void map_decision(struct selinux_map *map,
int security_mls_enabled(struct selinux_state *state)
{
- struct policydb *p = &state->ss->policydb;
+ struct policydb *p = &state->ss->active_set->policydb;
return p->mls_enabled;
}
@@ -733,7 +737,7 @@ static int security_validtrans_handle_fail(struct selinux_state *state,
struct context *tcontext,
u16 tclass)
{
- struct policydb *p = &state->ss->policydb;
+ struct policydb *p = &state->ss->active_set->policydb;
char *o = NULL, *n = NULL, *t = NULL;
u32 olen, nlen, tlen;
@@ -777,11 +781,11 @@ static int security_compute_validatetrans(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
if (!user)
- tclass = unmap_class(&state->ss->map, orig_tclass);
+ tclass = unmap_class(&state->ss->active_set->map, orig_tclass);
else
tclass = orig_tclass;
@@ -877,8 +881,8 @@ int security_bounded_transition(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
rc = -EINVAL;
old_context = sidtab_search(sidtab, old_sid);
@@ -1035,8 +1039,8 @@ void security_compute_xperms_decision(struct selinux_state *state,
if (!state->initialized)
goto allow;
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1052,7 +1056,7 @@ void security_compute_xperms_decision(struct selinux_state *state,
goto out;
}
- tclass = unmap_class(&state->ss->map, orig_tclass);
+ tclass = unmap_class(&state->ss->active_set->map, orig_tclass);
if (unlikely(orig_tclass && !tclass)) {
if (policydb->allow_unknown)
goto allow;
@@ -1124,8 +1128,8 @@ void security_compute_av(struct selinux_state *state,
if (!state->initialized)
goto allow;
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1145,7 +1149,7 @@ void security_compute_av(struct selinux_state *state,
goto out;
}
- tclass = unmap_class(&state->ss->map, orig_tclass);
+ tclass = unmap_class(&state->ss->active_set->map, orig_tclass);
if (unlikely(orig_tclass && !tclass)) {
if (policydb->allow_unknown)
goto allow;
@@ -1153,7 +1157,7 @@ void security_compute_av(struct selinux_state *state,
}
context_struct_compute_av(policydb, scontext, tcontext, tclass, avd,
xperms);
- map_decision(&state->ss->map, orig_tclass, avd,
+ map_decision(&state->ss->active_set->map, orig_tclass, avd,
policydb->allow_unknown);
out:
read_unlock(&state->ss->policy_rwlock);
@@ -1178,8 +1182,8 @@ void security_compute_av_user(struct selinux_state *state,
if (!state->initialized)
goto allow;
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1316,8 +1320,8 @@ static int security_sid_to_context_core(struct selinux_state *state,
goto out;
}
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
if (force)
context = sidtab_search_force(sidtab, sid);
else
@@ -1488,8 +1492,8 @@ static int security_context_to_sid_core(struct selinux_state *state,
goto out;
}
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
rc = string_to_context_struct(policydb, sidtab, scontext2,
scontext_len, &context, def_sid);
if (rc == -EINVAL && force) {
@@ -1576,7 +1580,7 @@ static int compute_sid_handle_invalid_context(
u16 tclass,
struct context *newcontext)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
char *s = NULL, *t = NULL, *n = NULL;
u32 slen, tlen, nlen;
@@ -1665,16 +1669,17 @@ static int security_compute_sid(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
if (kern) {
- tclass = unmap_class(&state->ss->map, orig_tclass);
+ tclass = unmap_class(&state->ss->active_set->map, orig_tclass);
sock = security_is_socket_class(orig_tclass);
} else {
+ struct selinux_map *amap = &state->ss->active_set->map;
tclass = orig_tclass;
- sock = security_is_socket_class(map_class(&state->ss->map,
+ sock = security_is_socket_class(map_class(amap,
tclass));
}
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1903,7 +1908,7 @@ static inline int convert_context_handle_invalid_context(
struct selinux_state *state,
struct context *context)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
char *s;
u32 len;
@@ -2071,9 +2076,9 @@ static int convert_context(u32 key,
goto out;
}
-static void security_load_policycaps(struct selinux_state *state)
+static void security_load_policycaps(struct selinux_state *state,
+ struct policydb *p)
{
- struct policydb *p = &state->ss->policydb;
unsigned int i;
struct ebitmap_node *node;
@@ -2107,47 +2112,47 @@ static int security_preserve_bools(struct selinux_state *state,
*/
int security_load_policy(struct selinux_state *state, void *data, size_t len)
{
- struct policydb *policydb;
- struct sidtab *sidtab;
- struct policydb *oldpolicydb, *newpolicydb;
- struct sidtab oldsidtab, newsidtab;
- struct selinux_mapping *oldmapping;
struct selinux_map newmap;
struct convert_context_args args;
u32 seqno;
int rc = 0;
+ struct selinux_ruleset *next_set, *old_set;
struct policy_file file = { data, len }, *fp = &file;
- oldpolicydb = kzalloc(2 * sizeof(*oldpolicydb), GFP_KERNEL);
- if (!oldpolicydb) {
+ next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
+ if (!next_set) {
rc = -ENOMEM;
goto out;
}
- newpolicydb = oldpolicydb + 1;
-
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ next_set->sidtab = kzalloc(sizeof(struct sidtab), GFP_KERNEL);
+ if (!next_set->sidtab) {
+ rc = -ENOMEM;
+ kfree(next_set);
+ goto out;
+ }
if (!state->initialized) {
- rc = policydb_read(policydb, fp);
+ old_set = state->ss->active_set;
+ rc = policydb_read(&next_set->policydb, fp);
if (rc)
goto out;
- policydb->len = len;
- rc = selinux_set_mapping(policydb, secclass_map,
- &state->ss->map);
+ next_set->policydb.len = len;
+ rc = selinux_set_mapping(&next_set->policydb, secclass_map,
+ &next_set->map);
if (rc) {
- policydb_destroy(policydb);
+ policydb_destroy(&next_set->policydb);
goto out;
}
- rc = policydb_load_isids(policydb, sidtab);
+ rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
if (rc) {
- policydb_destroy(policydb);
+ policydb_destroy(&next_set->policydb);
goto out;
}
- security_load_policycaps(state);
+ security_load_policycaps(state, &next_set->policydb);
+ state->ss->active_set = next_set;
state->initialized = 1;
seqno = ++state->ss->latest_granting;
selinux_complete_init();
@@ -2156,45 +2161,48 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
selinux_status_update_policyload(state, seqno);
selinux_netlbl_cache_invalidate();
selinux_xfrm_notify_policyload();
+ kfree(old_set->sidtab);
+ kfree(old_set);
goto out;
}
-
+ old_set = state->ss->active_set;
#if 0
sidtab_hash_eval(sidtab, "sids");
#endif
- rc = policydb_read(newpolicydb, fp);
+ rc = policydb_read(&next_set->policydb, fp);
if (rc)
goto out;
- newpolicydb->len = len;
+ next_set->policydb.len = len;
+
/* If switching between different policy types, log MLS status */
- if (policydb->mls_enabled && !newpolicydb->mls_enabled)
+ if (old_set->policydb.mls_enabled && !next_set->policydb.mls_enabled)
printk(KERN_INFO "SELinux: Disabling MLS support...\n");
- else if (!policydb->mls_enabled && newpolicydb->mls_enabled)
+ else if (!old_set->policydb.mls_enabled
+ && next_set->policydb.mls_enabled)
printk(KERN_INFO "SELinux: Enabling MLS support...\n");
-
- rc = policydb_load_isids(newpolicydb, &newsidtab);
+ rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
if (rc) {
printk(KERN_ERR "SELinux: unable to load the initial SIDs\n");
- policydb_destroy(newpolicydb);
+ policydb_destroy(&next_set->policydb);
goto out;
}
- rc = selinux_set_mapping(newpolicydb, secclass_map, &newmap);
+ rc = selinux_set_mapping(&next_set->policydb, secclass_map, &newmap);
if (rc)
goto err;
- rc = security_preserve_bools(state, newpolicydb);
+ rc = security_preserve_bools(state, &next_set->policydb);
if (rc) {
printk(KERN_ERR "SELinux: unable to preserve booleans\n");
goto err;
}
/* Clone the SID table. */
- sidtab_shutdown(sidtab);
+ sidtab_shutdown(old_set->sidtab);
- rc = sidtab_map(sidtab, clone_sid, &newsidtab);
+ rc = sidtab_map(old_set->sidtab, clone_sid, next_set->sidtab);
if (rc)
goto err;
@@ -2203,9 +2211,9 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
* in the new SID table.
*/
args.state = state;
- args.oldp = policydb;
- args.newp = newpolicydb;
- rc = sidtab_map(&newsidtab, convert_context, &args);
+ args.oldp = &old_set->policydb;
+ args.newp = &next_set->policydb;
+ rc = sidtab_map(next_set->sidtab, convert_context, &args);
if (rc) {
printk(KERN_ERR "SELinux: unable to convert the internal"
" representation of contexts in the new SID"
@@ -2213,48 +2221,43 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
goto err;
}
- /* Save the old policydb and SID table to free later. */
- memcpy(oldpolicydb, policydb, sizeof(*policydb));
- sidtab_set(&oldsidtab, sidtab);
+ next_set->map.mapping = newmap.mapping;
+ next_set->map.size = newmap.size;
/* Install the new policydb and SID table. */
write_lock_irq(&state->ss->policy_rwlock);
- memcpy(policydb, newpolicydb, sizeof(*policydb));
- sidtab_set(sidtab, &newsidtab);
- security_load_policycaps(state);
- oldmapping = state->ss->map.mapping;
- state->ss->map.mapping = newmap.mapping;
- state->ss->map.size = newmap.size;
+ security_load_policycaps(state, &next_set->policydb);
seqno = ++state->ss->latest_granting;
+ state->ss->active_set = next_set;
write_unlock_irq(&state->ss->policy_rwlock);
- /* Free the old policydb and SID table. */
- policydb_destroy(oldpolicydb);
- sidtab_destroy(&oldsidtab);
- kfree(oldmapping);
-
avc_ss_reset(state->avc, seqno);
selnl_notify_policyload(seqno);
selinux_status_update_policyload(state, seqno);
selinux_netlbl_cache_invalidate();
selinux_xfrm_notify_policyload();
+ /* Free the old policydb and SID table. */
+ policydb_destroy(&old_set->policydb);
+ sidtab_destroy(old_set->sidtab);
+ kfree(old_set->sidtab);
+ kfree(old_set->map.mapping);
+ kfree(old_set);
rc = 0;
goto out;
err:
kfree(newmap.mapping);
- sidtab_destroy(&newsidtab);
- policydb_destroy(newpolicydb);
-
+ sidtab_destroy(next_set->sidtab);
+ policydb_destroy(&next_set->policydb);
+ kfree(next_set);
out:
- kfree(oldpolicydb);
return rc;
}
size_t security_policydb_len(struct selinux_state *state)
{
- struct policydb *p = &state->ss->policydb;
+ struct policydb *p = &state->ss->active_set->policydb;
size_t len;
read_lock(&state->ss->policy_rwlock);
@@ -2280,8 +2283,8 @@ int security_port_sid(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
c = policydb->ocontexts[OCON_PORT];
while (c) {
@@ -2326,8 +2329,8 @@ int security_ib_pkey_sid(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
c = policydb->ocontexts[OCON_IBPKEY];
while (c) {
@@ -2372,8 +2375,8 @@ int security_ib_endport_sid(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
c = policydb->ocontexts[OCON_IBENDPORT];
while (c) {
@@ -2418,8 +2421,8 @@ int security_netif_sid(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
c = policydb->ocontexts[OCON_NETIF];
while (c) {
@@ -2483,8 +2486,8 @@ int security_node_sid(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
switch (domain) {
case AF_INET: {
@@ -2583,8 +2586,8 @@ int security_get_user_sids(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
context_init(&usercon);
@@ -2685,8 +2688,8 @@ static inline int __security_genfs_sid(struct selinux_state *state,
u16 orig_sclass,
u32 *sid)
{
- struct policydb *policydb = &state->ss->policydb;
- struct sidtab *sidtab = &state->ss->sidtab;
+ struct policydb *policydb = &state->ss->active_set->policydb;
+ struct sidtab *sidtab = state->ss->active_set->sidtab;
int len;
u16 sclass;
struct genfs *genfs;
@@ -2696,7 +2699,7 @@ static inline int __security_genfs_sid(struct selinux_state *state,
while (path[0] == '/' && path[1] == '/')
path++;
- sclass = unmap_class(&state->ss->map, orig_sclass);
+ sclass = unmap_class(&state->ss->active_set->map, orig_sclass);
*sid = SECINITSID_UNLABELED;
for (genfs = policydb->genfs; genfs; genfs = genfs->next) {
@@ -2771,8 +2774,8 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb)
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ policydb = &state->ss->active_set->policydb;
+ sidtab = state->ss->active_set->sidtab;
c = policydb->ocontexts[OCON_FSUSE];
while (c) {
@@ -2821,7 +2824,7 @@ int security_get_bools(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
+ policydb = &state->ss->active_set->policydb;
*names = NULL;
*values = NULL;
@@ -2866,53 +2869,86 @@ int security_get_bools(struct selinux_state *state,
int security_set_bools(struct selinux_state *state, int len, int *values)
{
- struct policydb *policydb;
int i, rc;
int lenp, seqno = 0;
struct cond_node *cur;
+ struct selinux_ruleset *next_set, *old_set = NULL;
+ void *storage;
+ size_t size;
- write_lock_irq(&state->ss->policy_rwlock);
+ next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
+ if (!next_set) {
+ rc = -ENOMEM;
+ goto errout;
+ }
+
+ rc = policydb_flattened_alloc(&state->ss->active_set->policydb,
+ &storage, &size);
+ if (rc) {
+ kfree(next_set);
+ goto errout;
+ }
- policydb = &state->ss->policydb;
+ write_lock_irq(&state->ss->policy_rwlock);
+ old_set = state->ss->active_set;
+ memcpy(next_set, old_set, sizeof(struct selinux_ruleset));
+ rc = policydb_copy(&old_set->policydb, &next_set->policydb,
+ &storage, size);
+ if (rc)
+ goto out;
rc = -EFAULT;
- lenp = policydb->p_bools.nprim;
+ lenp = next_set->policydb.p_bools.nprim;
if (len != lenp)
goto out;
for (i = 0; i < len; i++) {
- if (!!values[i] != policydb->bool_val_to_struct[i]->state) {
+ if (!!values[i] !=
+ next_set->policydb.bool_val_to_struct[i]->state) {
audit_log(current->audit_context, GFP_ATOMIC,
AUDIT_MAC_CONFIG_CHANGE,
"bool=%s val=%d old_val=%d auid=%u ses=%u",
- sym_name(policydb, SYM_BOOLS, i),
+ sym_name(&next_set->policydb, SYM_BOOLS, i),
!!values[i],
- policydb->bool_val_to_struct[i]->state,
+ next_set->policydb.bool_val_to_struct[i]->state,
from_kuid(&init_user_ns, audit_get_loginuid(current)),
audit_get_sessionid(current));
}
if (values[i])
- policydb->bool_val_to_struct[i]->state = 1;
+ next_set->policydb.bool_val_to_struct[i]->state = 1;
else
- policydb->bool_val_to_struct[i]->state = 0;
+ next_set->policydb.bool_val_to_struct[i]->state = 0;
}
- for (cur = policydb->cond_list; cur; cur = cur->next) {
- rc = evaluate_cond_node(policydb, cur);
+ for (cur = next_set->policydb.cond_list; cur; cur = cur->next) {
+ rc = evaluate_cond_node(&next_set->policydb, cur);
if (rc)
goto out;
}
seqno = ++state->ss->latest_granting;
+ state->ss->active_set = next_set;
rc = 0;
out:
- write_unlock_irq(&state->ss->policy_rwlock);
if (!rc) {
+ seqno = ++state->ss->latest_granting;
+ state->ss->active_set = next_set;
+ rc = 0;
+ write_unlock_irq(&state->ss->policy_rwlock);
avc_ss_reset(state->avc, seqno);
selnl_notify_policyload(seqno);
selinux_status_update_policyload(state, seqno);
selinux_xfrm_notify_policyload();
+ policydb_destroy(&old_set->policydb);
+ kfree(old_set);
+ } else {
+ printk(KERN_ERR "SELinux: %s failed %d\n", __func__, rc);
+ write_unlock_irq(&state->ss->policy_rwlock);
+ kfree(next_set);
}
+ policydb_flattened_free(storage);
+
+ errout:
return rc;
}
@@ -2925,7 +2961,7 @@ int security_get_bool_value(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
- policydb = &state->ss->policydb;
+ policydb = &state->ss->active_set->policydb;
rc = -EFAULT;
len = policydb->p_bools.nprim;
@@ -2977,8 +3013,8 @@ static int security_preserve_bools(struct selinux_state *state,
int security_sid_mls_copy(struct selinux_state *state,
u32 sid, u32 mls_sid, u32 *new_sid)
{
- struct policydb *policydb = &state->ss->policydb;
- struct sidtab *sidtab = &state->ss->sidtab;
+ struct policydb *policydb = &state->ss->active_set->policydb;
+ struct sidtab *sidtab = state->ss->active_set->sidtab;
struct context *context1;
struct context *context2;
struct context newcon;
@@ -3068,8 +3104,8 @@ int security_net_peersid_resolve(struct selinux_state *state,
u32 xfrm_sid,
u32 *peer_sid)
{
- struct policydb *policydb = &state->ss->policydb;
- struct sidtab *sidtab = &state->ss->sidtab;
+ struct policydb *policydb = &state->ss->active_set->policydb;
+ struct sidtab *sidtab = state->ss->active_set->sidtab;
int rc;
struct context *nlbl_ctx;
struct context *xfrm_ctx;
@@ -3146,7 +3182,7 @@ static int get_classes_callback(void *k, void *d, void *args)
int security_get_classes(struct selinux_state *state,
char ***classes, int *nclasses)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
int rc;
if (!state->initialized) {
@@ -3193,7 +3229,7 @@ static int get_permissions_callback(void *k, void *d, void *args)
int security_get_permissions(struct selinux_state *state,
char *class, char ***perms, int *nperms)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
int rc, i;
struct class_datum *match;
@@ -3239,12 +3275,12 @@ int security_get_permissions(struct selinux_state *state,
int security_get_reject_unknown(struct selinux_state *state)
{
- return state->ss->policydb.reject_unknown;
+ return state->ss->active_set->policydb.reject_unknown;
}
int security_get_allow_unknown(struct selinux_state *state)
{
- return state->ss->policydb.allow_unknown;
+ return state->ss->active_set->policydb.allow_unknown;
}
/**
@@ -3260,7 +3296,7 @@ int security_get_allow_unknown(struct selinux_state *state)
int security_policycap_supported(struct selinux_state *state,
unsigned int req_cap)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
int rc;
read_lock(&state->ss->policy_rwlock);
@@ -3288,7 +3324,7 @@ void selinux_audit_rule_free(void *vrule)
int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
{
struct selinux_state *state = &selinux_state;
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
struct selinux_audit_rule *tmprule;
struct role_datum *roledatum;
struct type_datum *typedatum;
@@ -3430,7 +3466,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule,
goto out;
}
- ctxt = sidtab_search(&state->ss->sidtab, sid);
+ ctxt = sidtab_search(state->ss->active_set->sidtab, sid);
if (unlikely(!ctxt)) {
WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n",
sid);
@@ -3592,8 +3628,8 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
struct netlbl_lsm_secattr *secattr,
u32 *sid)
{
- struct policydb *policydb = &state->ss->policydb;
- struct sidtab *sidtab = &state->ss->sidtab;
+ struct policydb *policydb = &state->ss->active_set->policydb;
+ struct sidtab *sidtab = state->ss->active_set->sidtab;
int rc;
struct context *ctx;
struct context ctx_new;
@@ -3661,7 +3697,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
int security_netlbl_sid_to_secattr(struct selinux_state *state,
u32 sid, struct netlbl_lsm_secattr *secattr)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
int rc;
struct context *ctx;
@@ -3671,7 +3707,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
rc = -ENOENT;
- ctx = sidtab_search(&state->ss->sidtab, sid);
+ ctx = sidtab_search(state->ss->active_set->sidtab, sid);
if (ctx == NULL)
goto out;
@@ -3700,7 +3736,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
int security_read_policy(struct selinux_state *state,
void **data, size_t *len)
{
- struct policydb *policydb = &state->ss->policydb;
+ struct policydb *policydb = &state->ss->active_set->policydb;
int rc;
struct policy_file fp;
diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
index 24c7bdcc8075..9219649c70ed 100644
--- a/security/selinux/ss/services.h
+++ b/security/selinux/ss/services.h
@@ -23,12 +23,18 @@ struct selinux_map {
u16 size; /* array size of mapping */
};
-struct selinux_ss {
- struct sidtab sidtab;
+/* sidtab is stored as a pointer. We can then choice to
+ * use the old pointer or create a new sittab.
+ */
+struct selinux_ruleset {
+ struct sidtab *sidtab;
struct policydb policydb;
+ struct selinux_map map;
+};
+struct selinux_ss {
+ struct selinux_ruleset *active_set; /* rcu pointer */
rwlock_t policy_rwlock;
u32 latest_granting;
- struct selinux_map map;
struct page *status_page;
struct mutex status_lock;
};
--
2.15.1
On 05/30/2018 10:10 AM, Peter Enderborg wrote:
> Holding the preempt_disable is very bad for low latency tasks
> such as audio and therefore we need to break out the rule-set dependent
> part from this disable. By using a RCU instead of rwlock we
> have an efficient locking and less preemption interference.
>
> Selinux uses a lot of read_locks. This patch replaces the rwlock
> with RCU that does not hold preempt_disable.
>
> Intel Xeon W3520 2.67 Ghz running FC27 with 4.15.0-rc9git (+measurement)
> I get preempt_disable of about 1.2ms in security_compute_av().
> With the patch I get 960us as the longest security_compute_av()
> without preempt disabeld. There are very much noise in the measurement
> but it is not likely a degrade.
>
> And the preempt_disable times is also very dependent on the selinux
> rule-set.
>
> In security_get_user_sids() we have two nested for-loops and the
> inner part calls sittab_context_to_sid() that calls
> sidtab_search_context() that has a for loop() over a while() where
> the loops is dependent on the rules.
>
> On the test system the average lookup time is 60us and does
> not change with the introduced RCU usage.
>
> The boolean change becomes a lot more heavy with this patch,
> but it is a very rare usage in compare with read only operations.
> The lock held during a policydb_copy is about 1ms on a XEON.
This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
>
> To use RCU the structure of policydb has to be accesses through a pointer.
> We need 5 patches to get there.
>
> [PATCH V3 1/5 selinux-next] selinux: Make allocation atomic in policydb objects functions.
> This patch change the allocation for policydb objects. They are in its own patch
> to make the complicated part easier to read.
>
> [PATCH V3 2/5 selinux-next] selinux: Introduce selinux_ruleset struct
> This makes the access for the rule evaluation going though a single pointer.
>
> [PATCH V3 3/5 selinux-next] selinux: sidtab_clone switch to use rwlock.
> We need to make sidtabs copys so this patch change the locks to a rwlock
> and create a copy function.
>
> [PATCH V3 4/5 selinux-next] selinux: seqno separation
> This patch adds separation of the read and write and uses
> the pointer to switch rule set. It uses seqno for error handling
> since there are a possibility to have multiple access.
>
> [PATCH V3 5/5 selinux-next] selinux: Switch to rcu read locks for avc_compute
> All the preparation is done so this patch do the change of locks to rcu.
>
> History:
> V1 rwsem
> V2 did not handle all policydb objects, solved with the policydb_copy
> did not handle sidtab for booleans, I think this one does however
> shutdown is not used but not removed.
>
>
(snip)
.
.
.
>
> -static void security_load_policycaps(struct selinux_state *state)
> +static void security_load_policycaps(struct selinux_state *state,
> + struct policydb *p)
> {
> - struct policydb *p = &state->ss->policydb;
> unsigned int i;
> struct ebitmap_node *node;
>
> @@ -2107,47 +2112,47 @@ static int security_preserve_bools(struct selinux_state *state,
> */
> int security_load_policy(struct selinux_state *state, void *data, size_t len)
> {
> - struct policydb *policydb;
> - struct sidtab *sidtab;
> - struct policydb *oldpolicydb, *newpolicydb;
> - struct sidtab oldsidtab, newsidtab;
> - struct selinux_mapping *oldmapping;
> struct selinux_map newmap;
> struct convert_context_args args;
> u32 seqno;
> int rc = 0;
> + struct selinux_ruleset *next_set, *old_set;
> struct policy_file file = { data, len }, *fp = &file;
>
> - oldpolicydb = kzalloc(2 * sizeof(*oldpolicydb), GFP_KERNEL);
> - if (!oldpolicydb) {
> + next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
> + if (!next_set) {
> rc = -ENOMEM;
> goto out;
> }
> - newpolicydb = oldpolicydb + 1;
> -
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + next_set->sidtab = kzalloc(sizeof(struct sidtab), GFP_KERNEL);
> + if (!next_set->sidtab) {
> + rc = -ENOMEM;
> + kfree(next_set);
How about moving these kfree(next_set) into the 'goto out' cleanup? The
effort has already been made to have a goto cleanup section in
security_load_policy). There is a lot of diff changes in here which is
hard to follow, and my worry is a kfree(next_set); could get missed, or
is not as easily maintainable as if the code was restructured to have a
single kfree(next_set); call for all 'goto out;' cleanup situations.
> + goto out;
> + }
>
> if (!state->initialized) {
> - rc = policydb_read(policydb, fp);
> + old_set = state->ss->active_set;
> + rc = policydb_read(&next_set->policydb, fp);
> if (rc)
> goto out;
>
> - policydb->len = len;
> - rc = selinux_set_mapping(policydb, secclass_map,
> - &state->ss->map);
> + next_set->policydb.len = len;
> + rc = selinux_set_mapping(&next_set->policydb, secclass_map,
> + &next_set->map);
> if (rc) {
> - policydb_destroy(policydb);
> + policydb_destroy(&next_set->policydb);
> goto out;
> }
>
> - rc = policydb_load_isids(policydb, sidtab);
> + rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
> if (rc) {
> - policydb_destroy(policydb);
> + policydb_destroy(&next_set->policydb);
> goto out;
> }
>
> - security_load_policycaps(state);
> + security_load_policycaps(state, &next_set->policydb);
> + state->ss->active_set = next_set;
> state->initialized = 1;
> seqno = ++state->ss->latest_granting;
> selinux_complete_init();
> @@ -2156,45 +2161,48 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> selinux_status_update_policyload(state, seqno);
> selinux_netlbl_cache_invalidate();
> selinux_xfrm_notify_policyload();
> + kfree(old_set->sidtab);
> + kfree(old_set);
> goto out;
> }
> -
> + old_set = state->ss->active_set;
> #if 0
> sidtab_hash_eval(sidtab, "sids");
> #endif
>
> - rc = policydb_read(newpolicydb, fp);
> + rc = policydb_read(&next_set->policydb, fp);
> if (rc)
> goto out;
>
> - newpolicydb->len = len;
> + next_set->policydb.len = len;
> +
> /* If switching between different policy types, log MLS status */
> - if (policydb->mls_enabled && !newpolicydb->mls_enabled)
> + if (old_set->policydb.mls_enabled && !next_set->policydb.mls_enabled)
> printk(KERN_INFO "SELinux: Disabling MLS support...\n");
> - else if (!policydb->mls_enabled && newpolicydb->mls_enabled)
> + else if (!old_set->policydb.mls_enabled
> + && next_set->policydb.mls_enabled)
> printk(KERN_INFO "SELinux: Enabling MLS support...\n");
> -
> - rc = policydb_load_isids(newpolicydb, &newsidtab);
> + rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to load the initial SIDs\n");
> - policydb_destroy(newpolicydb);
> + policydb_destroy(&next_set->policydb);
> goto out;
> }
>
> - rc = selinux_set_mapping(newpolicydb, secclass_map, &newmap);
> + rc = selinux_set_mapping(&next_set->policydb, secclass_map, &newmap);
> if (rc)
> goto err;
>
> - rc = security_preserve_bools(state, newpolicydb);
> + rc = security_preserve_bools(state, &next_set->policydb);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to preserve booleans\n");
> goto err;
> }
>
> /* Clone the SID table. */
> - sidtab_shutdown(sidtab);
> + sidtab_shutdown(old_set->sidtab);
>
> - rc = sidtab_map(sidtab, clone_sid, &newsidtab);
> + rc = sidtab_map(old_set->sidtab, clone_sid, next_set->sidtab);
> if (rc)
> goto err;
>
> @@ -2203,9 +2211,9 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> * in the new SID table.
> */
> args.state = state;
> - args.oldp = policydb;
> - args.newp = newpolicydb;
> - rc = sidtab_map(&newsidtab, convert_context, &args);
> + args.oldp = &old_set->policydb;
> + args.newp = &next_set->policydb;
> + rc = sidtab_map(next_set->sidtab, convert_context, &args);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to convert the internal"
> " representation of contexts in the new SID"
> @@ -2213,48 +2221,43 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> goto err;
> }
>
> - /* Save the old policydb and SID table to free later. */
> - memcpy(oldpolicydb, policydb, sizeof(*policydb));
> - sidtab_set(&oldsidtab, sidtab);
> + next_set->map.mapping = newmap.mapping;
> + next_set->map.size = newmap.size;
>
> /* Install the new policydb and SID table. */
> write_lock_irq(&state->ss->policy_rwlock);
> - memcpy(policydb, newpolicydb, sizeof(*policydb));
> - sidtab_set(sidtab, &newsidtab);
> - security_load_policycaps(state);
> - oldmapping = state->ss->map.mapping;
> - state->ss->map.mapping = newmap.mapping;
> - state->ss->map.size = newmap.size;
> + security_load_policycaps(state, &next_set->policydb);
> seqno = ++state->ss->latest_granting;
> + state->ss->active_set = next_set;
> write_unlock_irq(&state->ss->policy_rwlock);
>
> - /* Free the old policydb and SID table. */
> - policydb_destroy(oldpolicydb);
> - sidtab_destroy(&oldsidtab);
> - kfree(oldmapping);
> -
> avc_ss_reset(state->avc, seqno);
> selnl_notify_policyload(seqno);
> selinux_status_update_policyload(state, seqno);
> selinux_netlbl_cache_invalidate();
> selinux_xfrm_notify_policyload();
>
> + /* Free the old policydb and SID table. */
> + policydb_destroy(&old_set->policydb);
> + sidtab_destroy(old_set->sidtab);
> + kfree(old_set->sidtab);
> + kfree(old_set->map.mapping);
> + kfree(old_set);
> rc = 0;
> goto out;
>
> err:
> kfree(newmap.mapping);
> - sidtab_destroy(&newsidtab);
> - policydb_destroy(newpolicydb);
> -
> + sidtab_destroy(next_set->sidtab);
> + policydb_destroy(&next_set->policydb);
> + kfree(next_set);
> out:
> - kfree(oldpolicydb);
> return rc;
> }
>
> size_t security_policydb_len(struct selinux_state *state)
> {
> - struct policydb *p = &state->ss->policydb;
> + struct policydb *p = &state->ss->active_set->policydb;
> size_t len;
>
> read_lock(&state->ss->policy_rwlock);
> @@ -2280,8 +2283,8 @@ int security_port_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_PORT];
> while (c) {
> @@ -2326,8 +2329,8 @@ int security_ib_pkey_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_IBPKEY];
> while (c) {
> @@ -2372,8 +2375,8 @@ int security_ib_endport_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_IBENDPORT];
> while (c) {
> @@ -2418,8 +2421,8 @@ int security_netif_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_NETIF];
> while (c) {
> @@ -2483,8 +2486,8 @@ int security_node_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> switch (domain) {
> case AF_INET: {
> @@ -2583,8 +2586,8 @@ int security_get_user_sids(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> context_init(&usercon);
>
> @@ -2685,8 +2688,8 @@ static inline int __security_genfs_sid(struct selinux_state *state,
> u16 orig_sclass,
> u32 *sid)
> {
> - struct policydb *policydb = &state->ss->policydb;
> - struct sidtab *sidtab = &state->ss->sidtab;
> + struct policydb *policydb = &state->ss->active_set->policydb;
> + struct sidtab *sidtab = state->ss->active_set->sidtab;
> int len;
> u16 sclass;
> struct genfs *genfs;
> @@ -2696,7 +2699,7 @@ static inline int __security_genfs_sid(struct selinux_state *state,
> while (path[0] == '/' && path[1] == '/')
> path++;
>
> - sclass = unmap_class(&state->ss->map, orig_sclass);
> + sclass = unmap_class(&state->ss->active_set->map, orig_sclass);
> *sid = SECINITSID_UNLABELED;
>
> for (genfs = policydb->genfs; genfs; genfs = genfs->next) {
> @@ -2771,8 +2774,8 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb)
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_FSUSE];
> while (c) {
> @@ -2821,7 +2824,7 @@ int security_get_bools(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> + policydb = &state->ss->active_set->policydb;
>
> *names = NULL;
> *values = NULL;
> @@ -2866,53 +2869,86 @@ int security_get_bools(struct selinux_state *state,
>
> int security_set_bools(struct selinux_state *state, int len, int *values)
> {
> - struct policydb *policydb;
> int i, rc;
> int lenp, seqno = 0;
> struct cond_node *cur;
> + struct selinux_ruleset *next_set, *old_set = NULL;
> + void *storage;
> + size_t size;
>
> - write_lock_irq(&state->ss->policy_rwlock);
> + next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
> + if (!next_set) {
> + rc = -ENOMEM;
> + goto errout;
> + }
> +
> + rc = policydb_flattened_alloc(&state->ss->active_set->policydb,
> + &storage, &size);
> + if (rc) {
> + kfree(next_set);
I think this function, security_set_bools(), is another case where it
would be good to restructure the code where there is a single location
where kfree(next_set); is called for code readability and
maintainability. Or at the very least keeping the goto cleanup strategy
consistent; here, for normal 'goto out;' routines kfree(next_set); is
there in the 'out' block, but for 'goto errout;' routines it is not
there and kfree(next_set); must be called before-hand.
> + goto errout;
> + }
>
> - policydb = &state->ss->policydb;
> + write_lock_irq(&state->ss->policy_rwlock);
> + old_set = state->ss->active_set;
> + memcpy(next_set, old_set, sizeof(struct selinux_ruleset));
> + rc = policydb_copy(&old_set->policydb, &next_set->policydb,
> + &storage, size);
> + if (rc)
> + goto out;
>
> rc = -EFAULT;
> - lenp = policydb->p_bools.nprim;
> + lenp = next_set->policydb.p_bools.nprim;
> if (len != lenp)
> goto out;
>
> for (i = 0; i < len; i++) {
> - if (!!values[i] != policydb->bool_val_to_struct[i]->state) {
> + if (!!values[i] !=
> + next_set->policydb.bool_val_to_struct[i]->state) {
> audit_log(current->audit_context, GFP_ATOMIC,
> AUDIT_MAC_CONFIG_CHANGE,
> "bool=%s val=%d old_val=%d auid=%u ses=%u",
> - sym_name(policydb, SYM_BOOLS, i),
> + sym_name(&next_set->policydb, SYM_BOOLS, i),
> !!values[i],
> - policydb->bool_val_to_struct[i]->state,
> + next_set->policydb.bool_val_to_struct[i]->state,
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> audit_get_sessionid(current));
> }
> if (values[i])
> - policydb->bool_val_to_struct[i]->state = 1;
> + next_set->policydb.bool_val_to_struct[i]->state = 1;
> else
> - policydb->bool_val_to_struct[i]->state = 0;
> + next_set->policydb.bool_val_to_struct[i]->state = 0;
> }
>
> - for (cur = policydb->cond_list; cur; cur = cur->next) {
> - rc = evaluate_cond_node(policydb, cur);
> + for (cur = next_set->policydb.cond_list; cur; cur = cur->next) {
> + rc = evaluate_cond_node(&next_set->policydb, cur);
> if (rc)
> goto out;
> }
>
> seqno = ++state->ss->latest_granting;
> + state->ss->active_set = next_set;
> rc = 0;
> out:
> - write_unlock_irq(&state->ss->policy_rwlock);
> if (!rc) {
> + seqno = ++state->ss->latest_granting;
> + state->ss->active_set = next_set;
> + rc = 0;
Why would we want to set rc to 0 here and have the return value be 0
instead of the original rc value evaluated in the if() statement above?
> + write_unlock_irq(&state->ss->policy_rwlock);
> avc_ss_reset(state->avc, seqno);
> selnl_notify_policyload(seqno);
> selinux_status_update_policyload(state, seqno);
> selinux_xfrm_notify_policyload();
> + policydb_destroy(&old_set->policydb);
> + kfree(old_set);
> + } else {
> + printk(KERN_ERR "SELinux: %s failed %d\n", __func__, rc);
> + write_unlock_irq(&state->ss->policy_rwlock);
> + kfree(next_set);
> }
> + policydb_flattened_free(storage);
> +
> + errout:
> return rc;
> }
>
> @@
Thanks,
Jay
>
> +int sidtab_clone(struct sidtab *s, struct sidtab *d)
> +{
> + int i, rc = 0;
If s or d are NULL (see if() below), why would we want rc, the return
value, to be 0? How about defaulting rc to an error value (-EINVAL)?
> + struct sidtab_node *cur;
> +
> + if (!s || !d)
> + goto errout;
> +
> + read_lock(&s->lock);
> + for (i = 0; i < SIDTAB_SIZE; i++) {
> + cur = s->htable[i];
> + while (cur) {
> + if (cur->sid > SECINITSID_NUM)
> + rc = sidtab_insert(d, cur->sid, &cur->context);
> + if (rc)
> + goto out;
> + cur = cur->next;
> + }
> + }
> +out:
> + read_unlock(&s->lock);
> +errout:
> + return rc;
> +}
>
Thanks,
Jay
On 05/30/2018 11:22 PM, J Freyensee wrote:
>
>> +int sidtab_clone(struct sidtab *s, struct sidtab *d)
>> +{
>> + int i, rc = 0;
> If s or d are NULL (see if() below), why would we want rc, the return value, to be 0? How about defaulting rc to an error value (-EINVAL)?
Oops! Thanks, will fix in next set.
>> + struct sidtab_node *cur;
>> +
>> + if (!s || !d)
>> + goto errout;
>> +
>> + read_lock(&s->lock);
>> + for (i = 0; i < SIDTAB_SIZE; i++) {
>> + cur = s->htable[i];
>> + while (cur) {
>> + if (cur->sid > SECINITSID_NUM)
>> + rc = sidtab_insert(d, cur->sid, &cur->context);
>> + if (rc)
>> + goto out;
>> + cur = cur->next;
>> + }
>> + }
>> +out:
>> + read_unlock(&s->lock);
>> +errout:
>> + return rc;
>> +}
>>
> Thanks,
> Jay
>
On 05/30/2018 10:34 PM, Stephen Smalley wrote:
> On 05/30/2018 10:10 AM, Peter Enderborg wrote:
>> The boolean change becomes a lot more heavy with this patch,
>> but it is a very rare usage in compare with read only operations.
>> The lock held during a policydb_copy is about 1ms on a XEON.
> This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
> That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
> In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
That one did not make it, so I went for a other path. Make it simple, using the same serialisation that exist. That also make it easier to maintain.
We do not use the booleans in android since they are not allowed so im not aware of any use case where this administrative function are
used in such frequent manner that it would have an impact. And it must be some other large overhead with interprocess communication and
a multiple writes to sysfs during a boolean settings? However my concern is/was memory pressure, setting booleans will generate pressure
with lot of atomic allocation and large vmallocs. But my goal is the fast path for real time critical functions such as audio, and it will be a cost for
administrative tasks. On the xeon it takes about ~98 ms to run the security_set_bools compared to about ~8 ms without the overhead
of copying the policydb. About ~6 ms is rcu sync and ~8 ms is the same as the original update of selinux statuses, and about ~25 ms
is policydb_destroy() of the old copy.
>
On 05/31/2018 05:04 AM, peter enderborg wrote:
> On 05/30/2018 10:34 PM, Stephen Smalley wrote:
>> On 05/30/2018 10:10 AM, Peter Enderborg wrote:
>>> The boolean change becomes a lot more heavy with this patch,
>>> but it is a very rare usage in compare with read only operations.
>>> The lock held during a policydb_copy is about 1ms on a XEON.
>> This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
>> That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
>> In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
> That one did not make it, so I went for a other path. Make it simple, using the same serialisation that exist. That also make it easier to maintain.
> We do not use the booleans in android since they are not allowed so im not aware of any use case where this administrative function are
> used in such frequent manner that it would have an impact. And it must be some other large overhead with interprocess communication and
> a multiple writes to sysfs during a boolean settings? However my concern is/was memory pressure, setting booleans will generate pressure
> with lot of atomic allocation and large vmallocs.
Yes, that is also a concern. I would prefer to only duplicate the conditional policydb state as in KaiGai's patch.
Keeping temporary setting of booleans lightweight is desirable for other use cases than Android.
I'm also concerned by the implications of switching all of the allocations to atomic. KaiGai's patch did not take that approach either, and it obviously could make policy reload more prone to transient failures.
But my goal is the fast path for real time critical functions such as audio, and it will be a cost for
> administrative tasks. On the xeon it takes about ~98 ms to run the security_set_bools compared to about ~8 ms without the overhead
> of copying the policydb. About ~6 ms is rcu sync and ~8 ms is the same as the original update of selinux statuses, and about ~25 ms
> is policydb_destroy() of the old copy.
>>
On 05/31/2018 02:42 PM, Stephen Smalley wrote:
> On 05/31/2018 05:04 AM, peter enderborg wrote:
>> On 05/30/2018 10:34 PM, Stephen Smalley wrote:
>>> On 05/30/2018 10:10 AM, Peter Enderborg wrote:
>>>> The boolean change becomes a lot more heavy with this patch,
>>>> but it is a very rare usage in compare with read only operations.
>>>> The lock held during a policydb_copy is about 1ms on a XEON.
>>> This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
>>> That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
>>> In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
>> That one did not make it, so I went for a other path. Make it simple, using the same serialisation that exist. That also make it easier to maintain.
>> We do not use the booleans in android since they are not allowed so im not aware of any use case where this administrative function are
>> used in such frequent manner that it would have an impact. And it must be some other large overhead with interprocess communication and
>> a multiple writes to sysfs during a boolean settings? However my concern is/was memory pressure, setting booleans will generate pressure
>> with lot of atomic allocation and large vmallocs.
> Yes, that is also a concern. I would prefer to only duplicate the conditional policydb state as in KaiGai's patch.
> Keeping temporary setting of booleans lightweight is desirable for other use cases than Android.
>
> I'm also concerned by the implications of switching all of the allocations to atomic. KaiGai's patch did not take that approach either, and it obviously could make policy reload more prone to transient failures.
On the version 2 of the patchset you pointed out that I did a shallow copy, so I did a "deap" copy. As I see it the KaiGai cond_policydb_dup also do a shallow copy.
You dont happend to know exactly why KaiGai's patch never was accepted?
> But my goal is the fast path for real time critical functions such as audio, and it will be a cost for
>> administrative tasks. On the xeon it takes about ~98 ms to run the security_set_bools compared to about ~8 ms without the overhead
>> of copying the policydb. About ~6 ms is rcu sync and ~8 ms is the same as the original update of selinux statuses, and about ~25 ms
>> is policydb_destroy() of the old copy.
>
>
>
On 05/31/2018 10:12 AM, peter enderborg wrote:
> On 05/31/2018 02:42 PM, Stephen Smalley wrote:
>> On 05/31/2018 05:04 AM, peter enderborg wrote:
>>> On 05/30/2018 10:34 PM, Stephen Smalley wrote:
>>>> On 05/30/2018 10:10 AM, Peter Enderborg wrote:
>>>>> The boolean change becomes a lot more heavy with this patch,
>>>>> but it is a very rare usage in compare with read only operations.
>>>>> The lock held during a policydb_copy is about 1ms on a XEON.
>>>> This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
>>>> That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
>>>> In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
>>> That one did not make it, so I went for a other path. Make it simple, using the same serialisation that exist. That also make it easier to maintain.
>>> We do not use the booleans in android since they are not allowed so im not aware of any use case where this administrative function are
>>> used in such frequent manner that it would have an impact. And it must be some other large overhead with interprocess communication and
>>> a multiple writes to sysfs during a boolean settings? However my concern is/was memory pressure, setting booleans will generate pressure
>>> with lot of atomic allocation and large vmallocs.
>> Yes, that is also a concern. I would prefer to only duplicate the conditional policydb state as in KaiGai's patch.
>> Keeping temporary setting of booleans lightweight is desirable for other use cases than Android.
>>
>> I'm also concerned by the implications of switching all of the allocations to atomic. KaiGai's patch did not take that approach either, and it obviously could make policy reload more prone to transient failures.
>
> On the version 2 of the patchset you pointed out that I did a shallow copy, so I did a "deap" copy. As I see it the KaiGai cond_policydb_dup also do a shallow copy.
In your earlier patch set, you just did a memcpy of the policydb and then proceeded to mutate parts of the conditional policydb state, which would have modified the original too. KaiGai was performing a deep copy of the conditional portions of the policydb I believe.
> You dont happend to know exactly why KaiGai's patch never was accepted?
As I recall, there wasn't anything wrong with the code itself; he just wasn't satisfied that it ended up being a worthwhile tradeoff based on his own performance testing.
>
>> But my goal is the fast path for real time critical functions such as audio, and it will be a cost for
>>> administrative tasks. On the xeon it takes about ~98 ms to run the security_set_bools compared to about ~8 ms without the overhead
>>> of copying the policydb. About ~6 ms is rcu sync and ~8 ms is the same as the original update of selinux statuses, and about ~25 ms
>>> is policydb_destroy() of the old copy.
>>
>>
>>
On 05/31/2018 10:21 AM, Stephen Smalley wrote:
> On 05/31/2018 10:12 AM, peter enderborg wrote:
>> On 05/31/2018 02:42 PM, Stephen Smalley wrote:
>>> On 05/31/2018 05:04 AM, peter enderborg wrote:
>>>> On 05/30/2018 10:34 PM, Stephen Smalley wrote:
>>>>> On 05/30/2018 10:10 AM, Peter Enderborg wrote:
>>>>>> The boolean change becomes a lot more heavy with this patch,
>>>>>> but it is a very rare usage in compare with read only operations.
>>>>>> The lock held during a policydb_copy is about 1ms on a XEON.
>>>>> This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
>>>>> That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
>>>>> In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
>>>> That one did not make it, so I went for a other path. Make it simple, using the same serialisation that exist. That also make it easier to maintain.
>>>> We do not use the booleans in android since they are not allowed so im not aware of any use case where this administrative function are
>>>> used in such frequent manner that it would have an impact. And it must be some other large overhead with interprocess communication and
>>>> a multiple writes to sysfs during a boolean settings? However my concern is/was memory pressure, setting booleans will generate pressure
>>>> with lot of atomic allocation and large vmallocs.
>>> Yes, that is also a concern. I would prefer to only duplicate the conditional policydb state as in KaiGai's patch.
>>> Keeping temporary setting of booleans lightweight is desirable for other use cases than Android.
>>>
>>> I'm also concerned by the implications of switching all of the allocations to atomic. KaiGai's patch did not take that approach either, and it obviously could make policy reload more prone to transient failures.
>>
>> On the version 2 of the patchset you pointed out that I did a shallow copy, so I did a "deap" copy. As I see it the KaiGai cond_policydb_dup also do a shallow copy.
>
> In your earlier patch set, you just did a memcpy of the policydb and then proceeded to mutate parts of the conditional policydb state, which would have modified the original too. KaiGai was performing a deep copy of the conditional portions of the policydb I believe.
>
>> You dont happend to know exactly why KaiGai's patch never was accepted?
>
> As I recall, there wasn't anything wrong with the code itself; he just wasn't satisfied that it ended up being a worthwhile tradeoff based on his own performance testing.
His use case however was different - he was concerned with networking performance, and the introduction of the netnode/netport caches largely eliminated the policy rwlock as a concern there.
>
>>
>>> But my goal is the fast path for real time critical functions such as audio, and it will be a cost for
>>>> administrative tasks. On the xeon it takes about ~98 ms to run the security_set_bools compared to about ~8 ms without the overhead
>>>> of copying the policydb. About ~6 ms is rcu sync and ~8 ms is the same as the original update of selinux statuses, and about ~25 ms
>>>> is policydb_destroy() of the old copy.
>>>
>>>
>>>
On 05/31/2018 02:42 PM, Stephen Smalley wrote:
> On 05/31/2018 05:04 AM, peter enderborg wrote:
>> On 05/30/2018 10:34 PM, Stephen Smalley wrote:
>>> On 05/30/2018 10:10 AM, Peter Enderborg wrote:
>>>> The boolean change becomes a lot more heavy with this patch,
>>>> but it is a very rare usage in compare with read only operations.
>>>> The lock held during a policydb_copy is about 1ms on a XEON.
>>> This has a very substantial performance impact on setsebool, e.g. time setsebool httpd_can_sendmail=1.
>>> That's because you are doing a full vmalloc();policydb_write();policydb_read();vfree() sequence on it.
>>> In comparison, KaiGai's old attempt to replace the policy rwlock with RCU only duplicated the conditional policydb state (via a cond_policydb_dup) that he introduced. Is there a reason you couldn't use that approach?
>> That one did not make it, so I went for a other path. Make it simple, using the same serialisation that exist. That also make it easier to maintain.
>> We do not use the booleans in android since they are not allowed so im not aware of any use case where this administrative function are
>> used in such frequent manner that it would have an impact. And it must be some other large overhead with interprocess communication and
>> a multiple writes to sysfs during a boolean settings? However my concern is/was memory pressure, setting booleans will generate pressure
>> with lot of atomic allocation and large vmallocs.
> Yes, that is also a concern. I would prefer to only duplicate the conditional policydb state as in KaiGai's patch.
> Keeping temporary setting of booleans lightweight is desirable for other use cases than Android.
>
> I'm also concerned by the implications of switching all of the allocations to atomic. KaiGai's patch did not take that approach either, and it obviously could make policy reload more prone to transient failures.
It maybe not needed atomic at the time. But the duplication holds a rcu_read_lock so it need to be atomic now.
>
> But my goal is the fast path for real time critical functions such as audio, and it will be a cost for
>> administrative tasks. On the xeon it takes about ~98 ms to run the security_set_bools compared to about ~8 ms without the overhead
>> of copying the policydb. About ~6 ms is rcu sync and ~8 ms is the same as the original update of selinux statuses, and about ~25 ms
>> is policydb_destroy() of the old copy.
>
>
>
Hi Peter,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on pcmoore-selinux/next]
[also build test WARNING on v4.17-rc7]
[cannot apply to security/next next-20180531]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Peter-Enderborg/selinux-Make-allocation-atomic-in-policydb-objects-functions/20180601-205558
base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14,
from security/selinux/ss/policydb.c:33:
security/selinux/ss/policydb.c: In function 'policydb_flattened_alloc':
>> include/linux/kern_levels.h:5:18: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'size_t {aka unsigned int}' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
^~~~~~~~
>> security/selinux/ss/policydb.c:3548:10: note: in expansion of macro 'KERN_ERR'
printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
^~~~~~~~
security/selinux/ss/policydb.c:3548:50: note: format string is defined here
printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
~~^
%d
--
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14,
from security//selinux/ss/policydb.c:33:
security//selinux/ss/policydb.c: In function 'policydb_flattened_alloc':
>> include/linux/kern_levels.h:5:18: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'size_t {aka unsigned int}' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
^~~~~~~~
security//selinux/ss/policydb.c:3548:10: note: in expansion of macro 'KERN_ERR'
printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
^~~~~~~~
security//selinux/ss/policydb.c:3548:50: note: format string is defined here
printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
~~^
%d
vim +/KERN_ERR +3548 security/selinux/ss/policydb.c
3538
3539 int policydb_flattened_alloc(struct policydb *db, void **tmpbuf, size_t *size)
3540 {
3541 int rc = 0;
3542
3543 *size = db->len;
3544 *tmpbuf = vmalloc(*size);
3545
3546 if (!*tmpbuf) {
3547 rc = -ENOMEM;
> 3548 printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
3549 }
3550 return rc;
3551 }
3552
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Peter,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pcmoore-selinux/next]
[also build test ERROR on v4.17-rc7]
[cannot apply to security/next next-20180531]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Peter-Enderborg/selinux-Make-allocation-atomic-in-policydb-objects-functions/20180601-205558
base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sh
All error/warnings (new ones prefixed by >>):
security/selinux/ss/policydb.c: In function 'policydb_flattened_alloc':
>> security/selinux/ss/policydb.c:3544:12: error: implicit declaration of function 'vmalloc'; did you mean 'kvmalloc'? [-Werror=implicit-function-declaration]
*tmpbuf = vmalloc(*size);
^~~~~~~
kvmalloc
>> security/selinux/ss/policydb.c:3544:10: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
*tmpbuf = vmalloc(*size);
^
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14,
from security/selinux/ss/policydb.c:33:
include/linux/kern_levels.h:5:18: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'size_t {aka unsigned int}' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
^~~~~~~~
security/selinux/ss/policydb.c:3548:10: note: in expansion of macro 'KERN_ERR'
printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
^~~~~~~~
security/selinux/ss/policydb.c:3548:50: note: format string is defined here
printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
~~^
%d
security/selinux/ss/policydb.c: In function 'policydb_flattened_free':
>> security/selinux/ss/policydb.c:3555:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
vfree(tmpbuf);
^~~~~
kvfree
cc1: some warnings being treated as errors
vim +3544 security/selinux/ss/policydb.c
3538
3539 int policydb_flattened_alloc(struct policydb *db, void **tmpbuf, size_t *size)
3540 {
3541 int rc = 0;
3542
3543 *size = db->len;
> 3544 *tmpbuf = vmalloc(*size);
3545
3546 if (!*tmpbuf) {
3547 rc = -ENOMEM;
> 3548 printk(KERN_ERR "SELinux: vmalloc failed for %ld\n", *size);
3549 }
3550 return rc;
3551 }
3552
3553 int policydb_flattened_free(void *tmpbuf)
3554 {
> 3555 vfree(tmpbuf);
3556 return 0;
3557 }
3558
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation