linux-kernel - Re: PATCH [1/1]: audit: acquire creds selectively to reduce atomic op overhead

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110315173810.GA12775@suse.de>
Date:	Tue, 15 Mar 2011 10:38:10 -0700
From:	Tony Jones <tonyj@...e.de>
To:	David Howells <dhowells@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-audit@...hat.com,
	Eric Paris <eparis@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: PATCH [1/1]: audit: acquire creds selectively to reduce atomic
 op overhead

On Fri, Mar 11, 2011 at 04:33:52PM +0000, David Howells wrote:
> get_task_cred() and task_cred_xxxx() call __task_cred() which uses

Sorry, I thought you were referring to the remainder of auditsc.c

> It's possible that the credentials being used in audit_filter_rules() are
> incorrect under most circumstances and should be task->cred, not
> task->real_cred.

Agree. Also I believe it is safe to use tsk->cred directly as tsk == current 
or tsk is being created by copy_process.  

Eric, can you ACK/NACK?

The following patch corresponds to the above.

Tony
---


Commit c69e8d9c01db added calls to get_task_cred and put_cred in
audit_filter_rules.  Profiling with a large number of audit rules active on the
exit chain shows that we are spending upto 48% in this routine for syscall 
intensive tests, most of which is in the atomic ops.

The code should be accessing tsk->cred rather than tsk->real_cred.  Also, since
tsk is current (or tsk is being created by copy_process) direct access to 
tsk->cred is possible.

Signed-off-by: Tony Jones <tonyj@...e.de>
---

 kernel/auditsc.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f49a031..750c08ef 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -441,16 +441,21 @@ static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
 	return 0;
 }
 
-/* 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. */
+/*
+ * 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.
+ *
+ * Note: tsk==current or we are being indirectly called from copy_process()
+ * so direct access to tsk->cred is allowed.
+ */
 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)
 {
-	const struct cred *cred = get_task_cred(tsk);
+	const struct cred *cred = tsk->cred;
 	int i, j, need_sid = 1;
 	u32 sid;
 
@@ -637,10 +642,8 @@ static int audit_filter_rules(struct task_struct *tsk,
 			break;
 		}
 
-		if (!result) {
-			put_cred(cred);
+		if (!result)
 			return 0;
-		}
 	}
 
 	if (ctx) {
@@ -656,7 +659,6 @@ static int audit_filter_rules(struct task_struct *tsk,
 	case AUDIT_NEVER:    *state = AUDIT_DISABLED;	    break;
 	case AUDIT_ALWAYS:   *state = AUDIT_RECORD_CONTEXT; break;
 	}
-	put_cred(cred);
 	return 1;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/