Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335900 > unrolled thread

[PATCH 1/2] vfs: make sure struct filename->iname is word-aligned

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-02-16 23:50 +0100
    [PATCH 2/2] vfs: don't always include audit-specific members of struct filename Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-02-17 00:00 +0100
    Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-17 00:00 +0100
      Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-02-18 21:20 +0100
        Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Theodore Ts'o <tytso@mit.edu> - 2016-02-19 16:10 +0100

#1335900 — [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned

I noticed that offsetof(struct filename, iname) is actually 28 on 64
bit platforms, so we always pass an unaligned pointer to
strncpy_from_user. This is mostly a problem for those 64 bit platforms
without HAVE_EFFICIENT_UNALIGNED_ACCESS, but even on x86_64, unaligned
accesses carry a penalty, especially when done in a loop.

Let's try to ensure we always pass an aligned destination pointer to
strncpy_from_user. I considered making refcnt a long instead of doing
the union thing, and mostly ended up tossing a coin.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Cc'ing Linus, not because it's urgent in any way, but because he's
usually interested in strncpy_from_user and he can probably tell me
why this is completely immaterial.

 fs/namei.c         | 2 ++
 include/linux/fs.h | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index f624d132e01e..bd150fa799a2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -35,6 +35,7 @@
 #include <linux/fs_struct.h>
 #include <linux/posix_acl.h>
 #include <linux/hash.h>
+#include <linux/bug.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"
@@ -127,6 +128,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
 	struct filename *result;
 	char *kname;
 	int len;
+	BUILD_BUG_ON(offsetof(struct filename, iname) % sizeof(long) != 0);
 
 	result = audit_reusename(filename);
 	if (result)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ae681002100a..d522e6391855 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2245,7 +2245,10 @@ struct filename {
 	const char		*name;	/* pointer to actual string */
 	const __user char	*uptr;	/* original userland pointer */
 	struct audit_names	*aname;
-	int			refcnt;
+	union {
+		int		refcnt;
+		long		__padding;
+	};
 	const char		iname[];
 };
 
-- 
2.1.4

[toc] | [next] | [standalone]

#1335906 — [PATCH 2/2] vfs: don't always include audit-specific members of struct filename

The three members uptr, aname and refcnt are only used when
CONFIG_AUDITSYSCALL, a fact which is not obvious from the header file
or namei.c alone. So aside from eliminating a few useless instructions
in getname_flags and making EMBEDDED_NAME_MAX a little larger, this
patch also serves to document whoe the actual user of these members
is.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 fs/namei.c            | 10 ++++------
 include/linux/audit.h |  9 +++++++++
 include/linux/fs.h    |  2 ++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index bd150fa799a2..21410db25814 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -185,7 +185,6 @@ getname_flags(const char __user *filename, int flags, int *empty)
 		}
 	}
 
-	result->refcnt = 1;
 	/* The empty path is special. */
 	if (unlikely(!len)) {
 		if (empty)
@@ -196,8 +195,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
 		}
 	}
 
-	result->uptr = filename;
-	result->aname = NULL;
+	audit_init_filename(result, filename);
 	audit_getname(result);
 	return result;
 }
@@ -235,9 +233,7 @@ getname_kernel(const char * filename)
 		return ERR_PTR(-ENAMETOOLONG);
 	}
 	memcpy((char *)result->name, filename, len);
-	result->uptr = NULL;
-	result->aname = NULL;
-	result->refcnt = 1;
+	audit_init_filename(result, NULL);
 	audit_getname(result);
 
 	return result;
@@ -245,10 +241,12 @@ getname_kernel(const char * filename)
 
 void putname(struct filename *name)
 {
+#ifdef CONFIG_AUDITSYSCALL
 	BUG_ON(name->refcnt <= 0);
 
 	if (--name->refcnt > 0)
 		return;
+#endif
 
 	if (name->name != name->iname) {
 		__putname(name->name);
diff --git a/include/linux/audit.h b/include/linux/audit.h
index b40ed5df5542..7d7143674d85 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -232,6 +232,12 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
 extern void __audit_syscall_exit(int ret_success, long ret_value);
 extern struct filename *__audit_reusename(const __user char *uptr);
 extern void __audit_getname(struct filename *name);
+static inline void audit_init_filename(struct filename *name, const __user char *uptr)
+{
+	name->refcnt = 1;
+	name->aname = NULL;
+	name->uptr = uptr;
+}
 
 #define AUDIT_INODE_PARENT	1	/* dentry represents the parent */
 #define AUDIT_INODE_HIDDEN	2	/* audit record should be hidden */
@@ -459,6 +465,9 @@ static inline struct filename *audit_reusename(const __user char *name)
 }
 static inline void audit_getname(struct filename *name)
 { }
+static inline void audit_init_filename(struct filename *name, const __user char *uptr)
+{ }
+
 static inline void __audit_inode(struct filename *name,
 					const struct dentry *dentry,
 					unsigned int flags)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d522e6391855..df769f738695 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2243,12 +2243,14 @@ static inline int break_layout(struct inode *inode, bool wait)
 struct audit_names;
 struct filename {
 	const char		*name;	/* pointer to actual string */
+#ifdef CONFIG_AUDITSYSCALL
 	const __user char	*uptr;	/* original userland pointer */
 	struct audit_names	*aname;
 	union {
 		int		refcnt;
 		long		__padding;
 	};
+#endif
 	const char		iname[];
 };
 
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1335908

On Tue, Feb 16, 2016 at 11:49:24PM +0100, Rasmus Villemoes wrote:
> I noticed that offsetof(struct filename, iname) is actually 28 on 64
> bit platforms, so we always pass an unaligned pointer to
> strncpy_from_user. This is mostly a problem for those 64 bit platforms
> without HAVE_EFFICIENT_UNALIGNED_ACCESS, but even on x86_64, unaligned
> accesses carry a penalty, especially when done in a loop.
> 
> Let's try to ensure we always pass an aligned destination pointer to
> strncpy_from_user. I considered making refcnt a long instead of doing
> the union thing, and mostly ended up tossing a coin.

Why not swap it with the previous field, then?

[toc] | [prev] | [next] | [standalone]

#1337691

On Tue, Feb 16 2016, Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Feb 16, 2016 at 11:49:24PM +0100, Rasmus Villemoes wrote:
>> I noticed that offsetof(struct filename, iname) is actually 28 on 64
>> bit platforms, so we always pass an unaligned pointer to
>> strncpy_from_user. This is mostly a problem for those 64 bit platforms
>> without HAVE_EFFICIENT_UNALIGNED_ACCESS, but even on x86_64, unaligned
>> accesses carry a penalty, especially when done in a loop.
>> 
>> Let's try to ensure we always pass an aligned destination pointer to
>> strncpy_from_user. I considered making refcnt a long instead of doing
>> the union thing, and mostly ended up tossing a coin.
>
> Why not swap it with the previous field, then?

Sure, that would work as well. I don't really care how ->iname is pushed
out to offset 32, but I'd like to know if it's worth it.

Rasmus

[toc] | [prev] | [next] | [standalone]

#1338272

On Thu, Feb 18, 2016 at 09:10:21PM +0100, Rasmus Villemoes wrote:
> 
> Sure, that would work as well. I don't really care how ->iname is pushed
> out to offset 32, but I'd like to know if it's worth it.

Do you have access to one of these platforms where unaligned access is
really painful?  The usual thing is to benchmark something like "git
stat" which has to stat every single file in a repository's working
directory.  If you can't see it there, it seems unlikely you'd see it
anywhere else, yes?

					- Ted

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web