Skip to contentSkip to conversation

audit: Use a tracepoint for getname

PATCH In review Arnaldo Carvalho de Melo 4 participants 5 messages Sep 19, 2012 – Sep 20, 2012

Subsystems (3)
VFS Tracing Audit
lore
Share & tools
lore

Arnaldo Carvalho de Melo

author
#0 lore raw

PATCH 1/1 audit: Use a tracepoint for getname

Al, Eric,

Was this considered before? Acceptable?

- Arnaldo

---

Instead of an explicit hook only for audit, use a tracepoint, so that other users that need to know about filenames can hook there just like audit.

Based on an earlier patch by Thomas Gleixner that added the tracepoint but left the audit_getname call.

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 fs/namei.c                 |    5 ++++-
 include/linux/audit.h      |    6 +-----
 include/trace/events/vfs.h |   32 ++++++++++++++++++++++++++++++++
 init/Kconfig               |    2 +-
 kernel/audit.c             |   11 +++++++++++
 5 files changed, 49 insertions(+), 7 deletions(-)
 create mode 100644 include/trace/events/vfs.h
fs/namei.c +4 -1
index dd1ed1b..e1462d1 100644
@@ -39,6 +39,9 @@
 #include "internal.h"
 #include "mount.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/vfs.h>
+
 /* [Feb-1997 T. Schoebel-Theuer]
  * Fundamental changes in the pathname lookup mechanisms (namei)
  * were necessary because of omirr.  The reason is that omirr needs

@@ -141,7 +144,7 @@ static char *getname_flags(const char __user *filename, int flags, int *empty)
 
 	err = ERR_PTR(-ENAMETOOLONG);
 	if (likely(len < PATH_MAX)) {
-		audit_getname(result);
+		trace_getname(result);
 		return result;
 	}
include/linux/audit.h +1 -5
index 36abf2a..7ad39e0 100644
@@ -493,11 +493,7 @@ static inline void audit_syscall_exit(void *pt_regs)
 		__audit_syscall_exit(success, return_code);
 	}
 }
-static inline void audit_getname(const char *name)
-{
-	if (unlikely(!audit_dummy_context()))
-		__audit_getname(name);
-}
+
 static inline void audit_inode(const char *name, const struct dentry *dentry) {
 	if (unlikely(!audit_dummy_context()))
 		__audit_inode(name, dentry);
include/trace/events/vfs.h new file +32 -0
new file mode 100644
index 0000000..a6a5d1a
@@ -0,0 +1,32 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vfs
+
+#if !defined(_TRACE_VFS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_VFS_H_
+
+#include <linux/tracepoint.h>
+#include <linux/ftrace.h>
+
+TRACE_EVENT(getname,
+
+	TP_PROTO(const char *filename),
+
+	TP_ARGS(filename),
+
+	TP_STRUCT__entry(
+		__string(	filename, filename);
+	),
+
+	TP_fast_assign(
+		__assign_str(filename, filename);
+	),
+
+	TP_printk("vfs_getname %s", __get_str(filename))
+);
+
+#undef NO_DEV
+
+#endif /* _TRACE_VFS_H_ */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
init/Kconfig +1 -1
index af6c7f8..63413ea 100644
@@ -348,7 +348,7 @@ config TASK_IO_ACCOUNTING
 
 config AUDIT
 	bool "Auditing support"
-	depends on NET
+	depends on NET && TRACEPOINTS
 	help
 	  Enable auditing infrastructure that can be used with another
 	  kernel subsystem, such as SELinux (which requires this for
kernel/audit.c +11 -0
index ea3b7b6..99cb039 100644
@@ -64,6 +64,8 @@
 
 #include "audit.h"
 
+#include <trace/events/vfs.h>
+
 /* No auditing will take place until audit_initialized == AUDIT_INITIALIZED.
  * (Initialization happens after skb_init is called.) */
 #define AUDIT_DISABLED		-1

@@ -958,6 +960,12 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }
 
+static void audit_getname(void *ignore, const char *name)
+{
+	if (unlikely(!audit_dummy_context()))
+		__audit_getname(name);
+}
+
 /* Initialize audit support at boot time. */
 static int __init audit_init(void)
 {

@@ -978,6 +986,9 @@ static int __init audit_init(void)
 	else
 		audit_sock->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
 
+	if (register_trace_getname(audit_getname, NULL))
+		audit_panic("cannot register getname tracepoint");
+
 	skb_queue_head_init(&audit_skb_queue);
 	skb_queue_head_init(&audit_skb_hold_queue);
 	audit_initialized = AUDIT_INITIALIZED;
Signature · 1 line
1.7.1
Acked-by on v1 1/1
#1 lore raw
Quoted 16 lines · Arnaldo Carvalho de Melo

Al, Eric,

Was this considered before? Acceptable?

- Arnaldo

---

Instead of an explicit hook only for audit, use a tracepoint, so that other users that need to know about filenames can hook there just like audit.

Based on an earlier patch by Thomas Gleixner that added the tracepoint but left the audit_getname call.

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
Quoted 8 lines
---
 fs/namei.c                 |    5 ++++-
 include/linux/audit.h      |    6 +-----
 include/trace/events/vfs.h |   32 ++++++++++++++++++++++++++++++++
 init/Kconfig               |    2 +-
 kernel/audit.c             |   11 +++++++++++
 5 files changed, 49 insertions(+), 7 deletions(-)
 create mode 100644 include/trace/events/vfs.h

Nice generalization and we gain a useful tracepoint as well.

Acked-by Ingo Molnar <[email protected]>

Thanks,

Ingo

maintainer
#2 lore raw
In reply to Arnaldo Carvalho de Melo

cc'ing Jeff Layton who has recently done a lot of getname work and I want to make sure he sees this.

Quoted 13 lines · On Wed, 19 Sep 2012 15:56:59 -0700 Arnaldo Carvalho de Melo…

Al, Eric,

Was this considered before? Acceptable?

- Arnaldo

---

Instead of an explicit hook only for audit, use a tracepoint, so that other users that need to know about filenames can hook there just like audit.

kernel/audit.c static int __init audit_init(void) · line 994

Quoted 9 lines
kernel/audit.c +3 -0
@@ -978,6 +986,9 @@ static int __init audit_init(void)
 	else
 		audit_sock->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
 
+	if (register_trace_getname(audit_getname, NULL))
+		audit_panic("cannot register getname tracepoint");
+
 	skb_queue_head_init(&audit_skb_queue);
 	skb_queue_head_init(&audit_skb_hold_queue);
 	audit_initialized = AUDIT_INITIALIZED;

I think we need to just use panic instead of audit_panic. This early at boot userspace would not have been able to tell the kernel that audit_panic == panic nor would the box die later if userspace ask for that functionality. Instead the box would run but audit would be broken, which customers who want audit_panic == panic would be most upset about.

Otherwise, its good to me.

-Eric

maintainer
#3 lore raw
Quoted 6 lines · Eric Paris

I think we need to just use panic instead of audit_panic. This early at boot userspace would not have been able to tell the kernel that audit_panic == panic nor would the box die later if userspace ask for that functionality. Instead the box would run but audit would be broken, which customers who want audit_panic == panic would be most upset about.

Arnaldo, if you do convert it to panic, please denote that change in the change log and give Erics explanation as well (quote him if needed ;-)

Thanks,

-- Steve

author
#4 lore raw
In reply to Eric Paris

Em Thu, Sep 20, 2012 at 09:05:45AM -0400, Eric Paris escreveu:

Quoted 2 lines

cc'ing Jeff Layton who has recently done a lot of getname work and I want to make sure he sees this.

Thanks, will try with his patchset applied, but see below...

Quoted 6 lines
Quoted 4 lines · On Wed, 19 Sep 2012 15:56:59 -0700 Arnaldo Carvalho de Melo…
kernel/audit.c +0 -0
@@ -978,6 +986,9 @@ static int __init audit_init(void)
 	else
 		audit_sock->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
Quoted 8 lines

kernel/audit.c static int __init audit_init(void) · line 994

Quoted 6 lines
kernel/audit.c +3 -0
+	if (register_trace_getname(audit_getname, NULL))
+		audit_panic("cannot register getname tracepoint");
+
 	skb_queue_head_init(&audit_skb_queue);
 	skb_queue_head_init(&audit_skb_hold_queue);
 	audit_initialized = AUDIT_INITIALIZED;

I think we need to just use panic instead of audit_panic. This early

I don't have an opinion on that matter, just used what is done on the 'true' part of that other else, some lines above the register_trace_getname if:

        audit_sock = netlink_kernel_create(&init_net, NETLINK_AUDIT,
                                           THIS_MODULE, &cfg);
        if (!audit_sock)
                audit_panic("cannot initialize netlink socket");
        else
                audit_sock->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
Quoted 3 lines

at boot userspace would not have been able to tell the kernel that audit_panic == panic nor would the box die later if userspace ask for that functionality. Instead the box would run but audit would be

But then why audit_panic is called when netlink_kernel_create fails?

Quoted 4 lines

broken, which customers who want audit_panic == panic would be most upset about.

Otherwise, its good to me.

Thanks!

- Arnaldo