sched: Remove __rcu annotation from cred pointer

PATCH Superseded Amol Grover 2 participants 5 messages Feb 7, 2020 – Feb 13, 2020

Subsystems (3)

Scheduler Audit Credentials

lore

Share & tools

lore

Amol Grover

author

2020-02-07 18:05 UTC #0 lore raw

PATCH 1/3 sched: Remove __rcu annotation from cred pointer

task_struct::cred (subjective credentials) is *always* used task-synchronously, hence, does not require RCU semantics.

task_struct::real_cred (objective credentials) can be used in RCU context and its __rcu annotation is retained.

However, task_struct::cred and task_struct::real_cred *may* point to the same object, hence, the object pointed to by task_struct::cred *may* have RCU delayed freeing.

Suggested-by Jann Horn <[email protected]>

Co-developed-by Joel Fernandes (Google) <[email protected]>

Signed-off-by Joel Fernandes (Google) <[email protected]>

Signed-off-by Amol Grover <[email protected]>

---
 include/linux/sched.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

include/linux/sched.h +5 -2

index 716ad1d8d95e..39924e6e0cf2 100644
@@ -879,8 +879,11 @@ struct task_struct {
 	/* Objective and real subjective task credentials (COW): */
 	const struct cred __rcu		*real_cred;
 
-	/* Effective (overridable) subjective task credentials (COW): */
-	const struct cred __rcu		*cred;
+	/*
+	 * Effective (overridable) subjective task credentials (COW)
+	 * which is used task-synchronously
+	 */
+	const struct cred		*cred;
 
 #ifdef CONFIG_KEYS
 	/* Cached requested key. */

Signature · 1 line

2.24.1

Amol Grover

author

2020-02-07 18:05 UTC #1 lore raw

PATCH 2/3 cred: Do not use RCU primitives to access cred pointer

Since task_struct::cred can only be used task-synchronously, and is not visible to other threads under RCU context, we do not require RCU primitives to read/write to it and incur heavy barriers.

Suggested-by Jann Horn <[email protected]>

Co-developed-by Joel Fernandes (Google) <[email protected]>

Signed-off-by Joel Fernandes (Google) <[email protected]>

Signed-off-by Amol Grover <[email protected]>

---
 include/linux/cred.h | 5 ++---
 kernel/cred.c        | 6 +++---
 2 files changed, 5 insertions(+), 6 deletions(-)

include/linux/cred.h +2 -3

index 18639c069263..5973791e5fe4 100644
@@ -291,11 +291,10 @@ static inline void put_cred(const struct cred *_cred)
 /**
  * current_cred - Access the current task's subjective credentials
  *
- * Access the subjective credentials of the current task.  RCU-safe,
- * since nobody else can modify it.
+ * Access the subjective credentials of the current task.
  */
 #define current_cred() \
-	rcu_dereference_protected(current->cred, 1)
+	(current->cred)
 
 /**
  * current_real_cred - Access the current task's objective credentials

kernel/cred.c +3 -3

index 809a985b1793..3956c31d068d 100644
@@ -485,7 +485,7 @@ int commit_creds(struct cred *new)
 	if (new->user != old->user)
 		atomic_inc(&new->user->processes);
 	rcu_assign_pointer(task->real_cred, new);
-	rcu_assign_pointer(task->cred, new);
+	task->cred = new;
 	if (new->user != old->user)
 		atomic_dec(&old->user->processes);
 	alter_cred_subscribers(old, -2);

@@ -562,7 +562,7 @@ const struct cred *override_creds(const struct cred *new)
 	 */
 	get_new_cred((struct cred *)new);
 	alter_cred_subscribers(new, 1);
-	rcu_assign_pointer(current->cred, new);
+	current->cred = new;
 	alter_cred_subscribers(old, -1);
 
 	kdebug("override_creds() = %p{%d,%d}", old,

@@ -590,7 +590,7 @@ void revert_creds(const struct cred *old)
 	validate_creds(old);
 	validate_creds(override);
 	alter_cred_subscribers(old, 1);
-	rcu_assign_pointer(current->cred, old);
+	current->cred = old;
 	alter_cred_subscribers(override, -1);
 	put_cred(override);
 }

Signature · 1 line

2.24.1

Amol Grover

author

2020-02-07 18:05 UTC #2 lore raw

PATCH 3/3 auditsc: Do not use RCU primitive to read from cred pointer

In reply to Amol Grover

task_struct::cred is only used task-synchronously and does not require any RCU locks, hence, rcu_dereference_check is not required to read from it.

Suggested-by Jann Horn <[email protected]>

Co-developed-by Joel Fernandes (Google) <[email protected]>

Signed-off-by Joel Fernandes (Google) <[email protected]>

Signed-off-by Amol Grover <[email protected]>

---
 kernel/auditsc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

kernel/auditsc.c +5 ~~-10~~

index 4effe01ebbe2..d3510513cdd1 100644
@@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
 /* Determine if any context name data matches a rule's watch data */
 /* Compare a task_struct with an audit_rule.  Return 1 on match, 0
  * otherwise.
- *
- * If task_creation is true, this is an explicit indication that we are
- * filtering a task rule at task creation time.  This and tsk == current are
- * the only situations where tsk->cred may be accessed without an rcu read lock.
  */
 static int audit_filter_rules(struct task_struct *tsk,
 			      struct audit_krule *rule,
 			      struct audit_context *ctx,
 			      struct audit_names *name,
-			      enum audit_state *state,
-			      bool task_creation)
+			      enum audit_state *state)
 {
 	const struct cred *cred;
 	int i, need_sid = 1;
 	u32 sid;
 	unsigned int sessionid;
 
-	cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
+	cred = tsk->cred;
 
 	for (i = 0; i < rule->field_count; i++) {
 		struct audit_field *f = &rule->fields[i];

@@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
 	rcu_read_lock();
 	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
 		if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
-				       &state, true)) {
+				       &state)) {
 			if (state == AUDIT_RECORD_CONTEXT)
 				*key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
 			rcu_read_unlock();

@@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 	list_for_each_entry_rcu(e, list, list) {
 		if (audit_in_mask(&e->rule, ctx->major) &&
 		    audit_filter_rules(tsk, &e->rule, ctx, NULL,
-				       &state, false)) {
+				       &state)) {
 			rcu_read_unlock();
 			ctx->current_state = state;
 			return state;

@@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
 
 	list_for_each_entry_rcu(e, list, list) {
 		if (audit_in_mask(&e->rule, ctx->major) &&
-		    audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
+		    audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
 			ctx->current_state = state;
 			return 1;
 		}

Signature · 1 line

2.24.1

Paul Moore

maintainer Acked-by on v1 3/3

2020-02-11 15:19 UTC #3 lore raw

Re: [PATCH 3/3] auditsc: Do not use RCU primitive to read from cred pointer

Quoted 5 lines · Amol Grover

task_struct::cred is only used task-synchronously and does not require any RCU locks, hence, rcu_dereference_check is not required to read from it.

Quoted 7 lines

Suggested-by Jann Horn <[email protected]>
Co-developed-by Joel Fernandes (Google) <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>
Signed-off-by Amol Grover <[email protected]>
---
 kernel/auditsc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Considering the other changes in this patchset this change seems reasonable to me. I'm assuming you were intending this patchset to go in via some tree other than audit?

Acked-by Paul Moore <[email protected]>

Quoted 61 lines

kernel/auditsc.c +5 ~~-11~~

index 4effe01ebbe2..d3510513cdd1 100644
@@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
 /* Determine if any context name data matches a rule's watch data */
 /* Compare a task_struct with an audit_rule.  Return 1 on match, 0
  * otherwise.
- *
- * If task_creation is true, this is an explicit indication that we are
- * filtering a task rule at task creation time.  This and tsk == current are
- * the only situations where tsk->cred may be accessed without an rcu read lock.
  */
 static int audit_filter_rules(struct task_struct *tsk,
                              struct audit_krule *rule,
                              struct audit_context *ctx,
                              struct audit_names *name,
-                             enum audit_state *state,
-                             bool task_creation)
+                             enum audit_state *state)
 {
        const struct cred *cred;
        int i, need_sid = 1;
        u32 sid;
        unsigned int sessionid;
 
-       cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
+       cred = tsk->cred;
 
        for (i = 0; i < rule->field_count; i++) {
                struct audit_field *f = &rule->fields[i];

@@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
        rcu_read_lock();
        list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
                if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
-                                      &state, true)) {
+                                      &state)) {
                        if (state == AUDIT_RECORD_CONTEXT)
                                *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
                        rcu_read_unlock();

@@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
        list_for_each_entry_rcu(e, list, list) {
                if (audit_in_mask(&e->rule, ctx->major) &&
                    audit_filter_rules(tsk, &e->rule, ctx, NULL,
-                                      &state, false)) {
+                                      &state)) {
                        rcu_read_unlock();
                        ctx->current_state = state;
                        return state;

@@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
 
        list_for_each_entry_rcu(e, list, list) {
                if (audit_in_mask(&e->rule, ctx->major) &&
-                   audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
+                   audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
                        ctx->current_state = state;
                        return 1;
                }
--

2.24.1

Signature · 2 lines

paul moore
www.paul-moore.com

Amol Grover

author

2020-02-13 08:11 UTC #4 lore raw

Re: [PATCH 3/3] auditsc: Do not use RCU primitive to read from cred pointer

Quoted 14 lines · Paul Moore

Quoted 5 lines · Amol Grover
task_struct::cred is only used task-synchronously and does not require any RCU locks, hence, rcu_dereference_check is not required to read from it.
Quoted 7 lines
Suggested-by Jann Horn <[email protected]>
Co-developed-by Joel Fernandes (Google) <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>
Signed-off-by Amol Grover <[email protected]>
---
 kernel/auditsc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Quoted 4 lines

Considering the other changes in this patchset this change seems reasonable to me. I'm assuming you were intending this patchset to go in via some tree other than audit?

Yes, that's correct. Thank you for the ack!

Quoted 68 lines

Acked-by Paul Moore <[email protected]>

Quoted 61 lines

kernel/auditsc.c +5 ~~-11~~

index 4effe01ebbe2..d3510513cdd1 100644
@@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
 /* Determine if any context name data matches a rule's watch data */
 /* Compare a task_struct with an audit_rule.  Return 1 on match, 0
  * otherwise.
- *
- * If task_creation is true, this is an explicit indication that we are
- * filtering a task rule at task creation time.  This and tsk == current are
- * the only situations where tsk->cred may be accessed without an rcu read lock.
  */
 static int audit_filter_rules(struct task_struct *tsk,
                              struct audit_krule *rule,
                              struct audit_context *ctx,
                              struct audit_names *name,
-                             enum audit_state *state,
-                             bool task_creation)
+                             enum audit_state *state)
 {
        const struct cred *cred;
        int i, need_sid = 1;
        u32 sid;
        unsigned int sessionid;
 
-       cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
+       cred = tsk->cred;
 
        for (i = 0; i < rule->field_count; i++) {
                struct audit_field *f = &rule->fields[i];

@@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
        rcu_read_lock();
        list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
                if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
-                                      &state, true)) {
+                                      &state)) {
                        if (state == AUDIT_RECORD_CONTEXT)
                                *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
                        rcu_read_unlock();

@@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
        list_for_each_entry_rcu(e, list, list) {
                if (audit_in_mask(&e->rule, ctx->major) &&
                    audit_filter_rules(tsk, &e->rule, ctx, NULL,
-                                      &state, false)) {
+                                      &state)) {
                        rcu_read_unlock();
                        ctx->current_state = state;
                        return state;

@@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
 
        list_for_each_entry_rcu(e, list, list) {
                if (audit_in_mask(&e->rule, ctx->major) &&
-                   audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
+                   audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
                        ctx->current_state = state;
                        return 1;
                }
--

2.24.1

Signature · 2 lines

paul moore
www.paul-moore.com