[PATCH 1/1] audit: Use a tracepoint for getname

[PATCH 1/1] audit: Use a tracepoint for getname

[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 <acme@redhat.com>
---
 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

diff --git a/fs/namei.c b/fs/namei.c
index dd1ed1b..e1462d1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -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;
 	}
 
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 36abf2a..7ad39e0 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -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);
diff --git a/include/trace/events/vfs.h b/include/trace/events/vfs.h
new file mode 100644
index 0000000..a6a5d1a
--- /dev/null
+++ b/include/trace/events/vfs.h
@@ -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>
diff --git a/init/Kconfig b/init/Kconfig
index af6c7f8..63413ea 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -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
diff --git a/kernel/audit.c b/kernel/audit.c
index ea3b7b6..99cb039 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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;
-- 
1.7.1

Re: [PATCH 1/1] audit: Use a tracepoint for getname

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> 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 <acme@redhat.com>
> ---
>  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 <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 1/1] audit: Use a tracepoint for getname

[Eric Paris]

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


On Wed, 19 Sep 2012 15:56:59 -0700
Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> 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.

> @@ -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

Re: [PATCH 1/1] audit: Use a tracepoint for getname

[Steven Rostedt]

On Thu, 2012-09-20 at 09:05 -0400, Eric Paris wrote:

> 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

Re: [PATCH 1/1] audit: Use a tracepoint for getname

[Arnaldo Carvalho de Melo]

Em Thu, Sep 20, 2012 at 09:05:45AM -0400, Eric Paris escreveu:
> 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...
 
> On Wed, 19 Sep 2012 15:56:59 -0700
> Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > @@ -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

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;

> 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?

> broken, which customers who want audit_panic == panic would be most
> upset about.
> 
> Otherwise, its good to me.

Thanks!

- Arnaldo