headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 0/9] audit: overhaul audit_names handling to allow for retrying on path-based syscalls

I recently posted a set of patches to have the kernel retry the lookup
and call when path-based syscalls would ordinarily return ESTALE. Al
took a look and pointed out that this would break the fragile logic that
handles the audit_names for syscall auditing.

This patchset comprises a number of incremental changes that should make
it ok to retry on a path-based syscall. The main caveat is that the retry
mustn't redo the getname() on the strings involved.

Unfortunately, we don't have anything that really describes what the
correct behavior is for this stuff, so I'm shooting here for "no
discernable difference" on a retry.

This seems to do the right thing in the cases that I've tested; mostly
the normal case where things succeed or fail for some reason and where
the syscall is retried after an ESTALE error.

Review is of course appreciated. There are some fixes in here too for
some subtle bugs in the existing code. Some of these patches may also
help performance in some cases, but I haven't measured it for that.

I'd like to see this patchset go into 3.6 if at all possible.

Eric Paris (1):
  audit: make audit_compare_dname_path use parent_len helper

Jeff Layton (8):
  audit: remove unnecessary NULL ptr checks from do_path_lookup
  audit: pass in dentry to audit_copy_inode wherever possible
  audit: reverse arguments to audit_inode_child
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 4/9] audit: add a new "type" field to audit_names struct

For now, we just have two possibilities:

UNKNOWN: for a new audit_names record that we don't know anything about yet
NORMAL: for everything else

In later patches, we'll add other types so we can distinguish and update
records created under different circumstances.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 include/linux/audit.h |    5 +++++
 kernel/auditsc.c      |   15 ++++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 51eca54..7acbfc8 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
 <at>  <at>  -449,6 +449,11  <at>  <at>  struct audit_field {
 extern int __init audit_register_class(int class, unsigned *list);
 extern int audit_classify_syscall(int abi, unsigned syscall);
 extern int audit_classify_arch(int arch);
+
+/* audit_names->type values */
+#define	AUDIT_TYPE_UNKNOWN	0	/* we don't know yet */
+#define AUDIT_TYPE_NORMAL	1	/* a "normal" audit record */
+
 #ifdef CONFIG_AUDITSYSCALL
 /* These are defined in auditsc.c */
 				/* Public API */
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 1/9] audit: remove unnecessary NULL ptr checks from do_path_lookup

As best I can tell, whenever retval == 0, nd->path.dentry and nd->inode
are also non-NULL. Eliminate those checks and the superfluous
audit_context check.

Signed-off-by: Eric Paris <eparis <at> redhat.com>
Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/namei.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 7d69419..5f6a34d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
 <at>  <at>  -1812,12 +1812,8  <at>  <at>  static int do_path_lookup(int dfd, const char *name,
 	if (unlikely(retval == -ESTALE))
 		retval = path_lookupat(dfd, name, flags | LOOKUP_REVAL, nd);

-	if (likely(!retval)) {
-		if (unlikely(!audit_dummy_context())) {
-			if (nd->path.dentry && nd->inode)
-				audit_inode(name, nd->path.dentry);
-		}
-	}
+	if (likely(!retval))
+		audit_inode(name, nd->path.dentry);
 	return retval;
 }

--

--
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 8/9] audit: optimize audit_compare_dname_path

In the cases where we already know the length of the parent, pass it as
a parm so we don't need to recompute it. In the cases where we don't
know the length, pass in AUDIT_NAME_FULL (-1) to indicate that it should
be determined.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 kernel/audit.h       |    5 ++++-
 kernel/audit_watch.c |    3 ++-
 kernel/auditfilter.c |   16 +++++++++++-----
 kernel/auditsc.c     |    8 +++-----
 4 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index ee31316..34af33c 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
 <at>  <at>  -74,10 +74,13  <at>  <at>  static inline int audit_hash_ino(u32 ino)
 	return (ino & (AUDIT_INODE_BUCKETS-1));
 }

+/* Indicates that audit should log the full pathname. */
+#define AUDIT_NAME_FULL -1
+
 extern int audit_match_class(int class, unsigned syscall);
 extern int audit_comparator(const u32 left, const u32 op, const u32 right);
 extern int parent_len(const char *path);
-extern int audit_compare_dname_path(const char *dname, const char *path);
+extern int audit_compare_dname_path(const char *dname, const char *path, int plen);
 extern struct sk_buff *	    audit_make_reply(int pid, int seq, int type,
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 9/9] audit: overhaul __audit_inode_child to accomodate retrying

In order to accomodate retrying path-based syscalls, we need to add a
new "type" argument to audit_inode_child. This will tell us whether
we're looking for a child entry that represents a create or a delete.

If we find a parent, don't automatically assume that we need to create a
new entry. Instead, use the information we have to try to find an
existing entry first. Update it if one is found and create a new one if
not.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/btrfs/ioctl.c         |    2 +-
 fs/namei.c               |    2 +-
 include/linux/audit.h    |   12 ++++++---
 include/linux/fsnotify.h |    8 +++---
 kernel/auditsc.c         |   57 ++++++++++++++++++++++++---------------------
 5 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 90c67b0..b638ac5 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
 <at>  <at>  -592,7 +592,7  <at>  <at>  static int btrfs_may_delete(struct inode *dir,struct dentry *victim,int isdir)
 		return -ENOENT;

 	BUG_ON(victim->d_parent->d_inode != dir);
-	audit_inode_child(dir, victim);
+	audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);

 	error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 2/9] audit: pass in dentry to audit_copy_inode wherever possible

In some cases, we were passing in NULL even when we have a dentry.

Reported-by: Eric Paris <eparis <at> redhat.com>
Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 kernel/auditsc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4b96415..5c45b9b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
 <at>  <at>  -2226,7 +2226,7  <at>  <at>  void __audit_inode_child(const struct dentry *dentry,
 		if (!strcmp(dname, n->name) ||
 		     !audit_compare_dname_path(dname, n->name, &dirlen)) {
 			if (inode)
-				audit_copy_inode(n, NULL, inode);
+				audit_copy_inode(n, dentry, inode);
 			else
 				n->ino = (unsigned long)-1;
 			found_child = n->name;
 <at>  <at>  -2258,7 +2258,7  <at>  <at>  add_names:
 		}

 		if (inode)
-			audit_copy_inode(n, NULL, inode);
+			audit_copy_inode(n, dentry, inode);
 	}
 }
 EXPORT_SYMBOL_GPL(__audit_inode_child);
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 6/9] audit: remove dirlen argument to audit_compare_dname_path

All the callers set this to NULL now.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 kernel/audit.h       |    3 +--
 kernel/audit_watch.c |    2 +-
 kernel/auditfilter.c |    6 +-----
 kernel/auditsc.c     |    4 ++--
 4 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index 276ca88..ee31316 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
 <at>  <at>  -77,8 +77,7  <at>  <at>  static inline int audit_hash_ino(u32 ino)
 extern int audit_match_class(int class, unsigned syscall);
 extern int audit_comparator(const u32 left, const u32 op, const u32 right);
 extern int parent_len(const char *path);
-extern int audit_compare_dname_path(const char *dname, const char *path,
-				    int *dirlen);
+extern int audit_compare_dname_path(const char *dname, const char *path);
 extern struct sk_buff *	    audit_make_reply(int pid, int seq, int type,
 					     int done, int multi,
 					     const void *payload, int size);
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index e683869..44f3b31 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
 <at>  <at>  -265,7 +265,7  <at>  <at>  static void audit_update_watch(struct audit_parent *parent,
 	/* Run all of the watches on this parent looking for the one that
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 7/9] audit: make audit_compare_dname_path use parent_len helper

From: Eric Paris <eparis <at> redhat.com>

Signed-off-by: Eric Paris <eparis <at> redhat.com>
---
 kernel/auditfilter.c |   27 +++++++--------------------
 1 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f9c48d0..f47ba18 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
 <at>  <at>  -1232,32 +1232,19  <at>  <at>  int parent_len(const char *path)
  * return of 0 indicates a match. */
 int audit_compare_dname_path(const char *dname, const char *path)
 {
-	int dlen, plen;
+	int dlen, pathlen, parentlen;
 	const char *p;

-	if (!dname || !path)
-		return 1;
-
 	dlen = strlen(dname);
-	plen = strlen(path);
-	if (plen < dlen)
+	pathlen = strlen(path);
+	if (pathlen < dlen)
 		return 1;

-	/* disregard trailing slashes */
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 5/9] audit: set the name_len in audit_inode for parent lookups

Currently, this gets set mostly by happenstance when we call into
audit_inode_child. While that might be a little more efficient, it seems
wrong. If the syscall ends up failing before audit_inode_child ever gets
called, then you'll have an audit_names record that shows the full path
but has the parent inode info attached.

Fix this by passing in a parent flag when we call audit_inode that gets
set to the value of LOOKUP_PARENT. We can then fix up the pathname for
the audit entry correctly from the get-go.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/namei.c            |   12 ++++++------
 fs/open.c             |    4 ++--
 fs/xattr.c            |    8 ++++----
 include/linux/audit.h |   13 ++++++++-----
 ipc/mqueue.c          |    8 ++++----
 kernel/audit.h        |    1 +
 kernel/auditfilter.c  |   30 ++++++++++++++++++++++++++++++
 kernel/auditsc.c      |   41 +++++++++++++++++++++++++++++------------
 8 files changed, 84 insertions(+), 33 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 8539f8f..8c1b3cd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
 <at>  <at>  -1813,7 +1813,7  <at>  <at>  static int do_path_lookup(int dfd, const char *name,
 		retval = path_lookupat(dfd, name, flags | LOOKUP_REVAL, nd);

 	if (likely(!retval))
(Continue reading)

Permalink | Reply |
headers
Jeff Layton | 26 Jun 2012 18:35
Picon
Favicon

[PATCH v4 3/9] audit: reverse arguments to audit_inode_child

Most of the callers get called with an inode and dentry in the reverse
order. The compiler then has to reshuffle the arg registers and/or
stack in order to pass them on to audit_inode_child.

Reverse those arguments for a micro-optimization.

Reported-by: Eric Paris <eparis <at> redhat.com>
Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/btrfs/ioctl.c         |    2 +-
 fs/namei.c               |    2 +-
 include/linux/audit.h    |   14 +++++++-------
 include/linux/fsnotify.h |    8 ++++----
 kernel/auditsc.c         |    8 ++++----
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0e92e57..90c67b0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
 <at>  <at>  -592,7 +592,7  <at>  <at>  static int btrfs_may_delete(struct inode *dir,struct dentry *victim,int isdir)
 		return -ENOENT;

 	BUG_ON(victim->d_parent->d_inode != dir);
-	audit_inode_child(victim, dir);
+	audit_inode_child(dir, victim);

 	error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
 	if (error)
diff --git a/fs/namei.c b/fs/namei.c
(Continue reading)

Permalink | Reply |

Gmane