[PATCH] fs: fix lock initialization

[PATCH] fs: fix lock initialization

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

locks_alloc_lock() assumed that the allocated struct file_lock is
already initialized to zero members.  This is only true for the first
allocation of the structure, after reuse some of the members will have
random values.

This will for example result in passing random fl_start values to
userspace in fuse for FL_FLOCK locks, which is an information leak at
best.

Fix by reinitializing those members which may be non-zero after freeing.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: stable@kernel.org
---
 fs/locks.c |   30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

Index: linux-2.6/fs/locks.c
===================================================================
--- linux-2.6.orig/fs/locks.c	2011-07-04 17:06:01.000000000 +0200
+++ linux-2.6/fs/locks.c	2011-07-04 17:06:04.000000000 +0200
@@ -160,10 +160,28 @@ EXPORT_SYMBOL_GPL(unlock_flocks);
 
 static struct kmem_cache *filelock_cache __read_mostly;
 
+static void locks_init_lock_always(struct file_lock *fl)
+{
+	fl->fl_next = NULL;
+	fl->fl_fasync = NULL;
+	fl->fl_owner = NULL;
+	fl->fl_pid = 0;
+	fl->fl_nspid = NULL;
+	fl->fl_file = NULL;
+	fl->fl_flags = 0;
+	fl->fl_type = 0;
+	fl->fl_start = fl->fl_end = 0;
+}
+
 /* Allocate an empty lock structure. */
 struct file_lock *locks_alloc_lock(void)
 {
-	return kmem_cache_alloc(filelock_cache, GFP_KERNEL);
+	struct file_lock *fl = kmem_cache_alloc(filelock_cache, GFP_KERNEL);
+
+	if (fl)
+		locks_init_lock_always(fl);
+
+	return fl;
 }
 EXPORT_SYMBOL_GPL(locks_alloc_lock);
 
@@ -200,17 +218,9 @@ void locks_init_lock(struct file_lock *f
 	INIT_LIST_HEAD(&fl->fl_link);
 	INIT_LIST_HEAD(&fl->fl_block);
 	init_waitqueue_head(&fl->fl_wait);
-	fl->fl_next = NULL;
-	fl->fl_fasync = NULL;
-	fl->fl_owner = NULL;
-	fl->fl_pid = 0;
-	fl->fl_nspid = NULL;
-	fl->fl_file = NULL;
-	fl->fl_flags = 0;
-	fl->fl_type = 0;
-	fl->fl_start = fl->fl_end = 0;
 	fl->fl_ops = NULL;
 	fl->fl_lmops = NULL;
+	locks_init_lock_always(fl);
 }
 
 EXPORT_SYMBOL(locks_init_lock);

Re: [PATCH] fs: fix lock initialization

[Linus Torvalds]

On Wed, Jul 6, 2011 at 3:33 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> locks_alloc_lock() assumed that the allocated struct file_lock is
> already initialized to zero members.  This is only true for the first
> allocation of the structure, after reuse some of the members will have
> random values.

Ugh. Code that depends on SLAB initializers is broken.

The reason for those initializers are traditionally "better cache
behavior" where you don't need to initialize everything at allocation
time, but the whole concept is almost invariably a disaster. This is a
prime example of it.

I also doubt the cache value - what happens is that you tend to touch
_part_ of the cachelines instead of just clearing the whole thing,
which is quite often not at all more efficient. And even when you are
able to avoid touching a cacheline entirely, most of the time it just
moves the cache miss cost elsewhere to the user.

There are some valid uses of initializers (eg code that depends on RCU
freeing and very much is about not re-initializing a lock), but they
really are about those kinds special cases. We should try to encourage
people to limit initializers to *only* those kinds of things.

Thanks for noticing,

               Linus

Re: [PATCH] fs: fix lock initialization

[J. Bruce Fields]

On Wed, Jul 06, 2011 at 12:33:55PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> locks_alloc_lock() assumed that the allocated struct file_lock is
> already initialized to zero members.  This is only true for the first
> allocation of the structure, after reuse some of the members will have
> random values.
> 
> This will for example result in passing random fl_start values to
> userspace in fuse for FL_FLOCK locks, which is an information leak at
> best.
> 
> Fix by reinitializing those members which may be non-zero after freeing.

Could you also just get rid of init_once() while you're at it?

--b.

> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: stable@kernel.org
> ---
>  fs/locks.c |   30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> Index: linux-2.6/fs/locks.c
> ===================================================================
> --- linux-2.6.orig/fs/locks.c	2011-07-04 17:06:01.000000000 +0200
> +++ linux-2.6/fs/locks.c	2011-07-04 17:06:04.000000000 +0200
> @@ -160,10 +160,28 @@ EXPORT_SYMBOL_GPL(unlock_flocks);
>  
>  static struct kmem_cache *filelock_cache __read_mostly;
>  
> +static void locks_init_lock_always(struct file_lock *fl)
> +{
> +	fl->fl_next = NULL;
> +	fl->fl_fasync = NULL;
> +	fl->fl_owner = NULL;
> +	fl->fl_pid = 0;
> +	fl->fl_nspid = NULL;
> +	fl->fl_file = NULL;
> +	fl->fl_flags = 0;
> +	fl->fl_type = 0;
> +	fl->fl_start = fl->fl_end = 0;
> +}
> +
>  /* Allocate an empty lock structure. */
>  struct file_lock *locks_alloc_lock(void)
>  {
> -	return kmem_cache_alloc(filelock_cache, GFP_KERNEL);
> +	struct file_lock *fl = kmem_cache_alloc(filelock_cache, GFP_KERNEL);
> +
> +	if (fl)
> +		locks_init_lock_always(fl);
> +
> +	return fl;
>  }
>  EXPORT_SYMBOL_GPL(locks_alloc_lock);
>  
> @@ -200,17 +218,9 @@ void locks_init_lock(struct file_lock *f
>  	INIT_LIST_HEAD(&fl->fl_link);
>  	INIT_LIST_HEAD(&fl->fl_block);
>  	init_waitqueue_head(&fl->fl_wait);
> -	fl->fl_next = NULL;
> -	fl->fl_fasync = NULL;
> -	fl->fl_owner = NULL;
> -	fl->fl_pid = 0;
> -	fl->fl_nspid = NULL;
> -	fl->fl_file = NULL;
> -	fl->fl_flags = 0;
> -	fl->fl_type = 0;
> -	fl->fl_start = fl->fl_end = 0;
>  	fl->fl_ops = NULL;
>  	fl->fl_lmops = NULL;
> +	locks_init_lock_always(fl);
>  }
>  
>  EXPORT_SYMBOL(locks_init_lock);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: fix lock initialization

[Matthew Wilcox]

On Wed, Jul 06, 2011 at 10:40:44AM -0700, Linus Torvalds wrote:
> Ugh. Code that depends on SLAB initializers is broken.
> 
> The reason for those initializers are traditionally "better cache
> behavior" where you don't need to initialize everything at allocation
> time, but the whole concept is almost invariably a disaster. This is a
> prime example of it.

It'd be good to have that documented somewhere.  When I wrote this code
(a decade ago), that wasn't my understanding.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] fs: fix lock initialization

[Miklos Szeredi]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Wed, Jul 06, 2011 at 12:33:55PM +0200, Miklos Szeredi wrote:
>> From: Miklos Szeredi <mszeredi@suse.cz>
>> 
>> locks_alloc_lock() assumed that the allocated struct file_lock is
>> already initialized to zero members.  This is only true for the first
>> allocation of the structure, after reuse some of the members will have
>> random values.
>> 
>> This will for example result in passing random fl_start values to
>> userspace in fuse for FL_FLOCK locks, which is an information leak at
>> best.
>> 
>> Fix by reinitializing those members which may be non-zero after freeing.
>
> Could you also just get rid of init_once() while you're at it?

Sure.  Updated patch below.

Thanks,
Miklos
----

Subject: fs: fix lock initialization

From: Miklos Szeredi <mszeredi@suse.cz>

locks_alloc_lock() assumed that the allocated struct file_lock is
already initialized to zero members.  This is only true for the first
allocation of the structure, after reuse some of the members will have
random values.

This will for example result in passing random fl_start values to
userspace in fuse for FL_FLOCK locks, which is an information leak at
best.

Remove SLAB initialization entirely, as suggested by Bruce and Linus.
Allocate with __GFP_ZERO instead and only initilaize list heads.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: stable@kernel.org
---
 fs/locks.c |   45 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

Index: linux-2.6/fs/locks.c
===================================================================
--- linux-2.6.orig/fs/locks.c	2011-07-06 12:21:33.000000000 +0200
+++ linux-2.6/fs/locks.c	2011-07-07 12:15:19.000000000 +0200
@@ -160,10 +160,22 @@ EXPORT_SYMBOL_GPL(unlock_flocks);
 
 static struct kmem_cache *filelock_cache __read_mostly;
 
+static void locks_init_lock_heads(struct file_lock *fl)
+{
+	INIT_LIST_HEAD(&fl->fl_link);
+	INIT_LIST_HEAD(&fl->fl_block);
+	init_waitqueue_head(&fl->fl_wait);
+}
+
 /* Allocate an empty lock structure. */
 struct file_lock *locks_alloc_lock(void)
 {
-	return kmem_cache_alloc(filelock_cache, GFP_KERNEL);
+	struct file_lock *fl = kmem_cache_zalloc(filelock_cache, GFP_KERNEL);
+
+	if (fl)
+		locks_init_lock_heads(fl);
+
+	return fl;
 }
 EXPORT_SYMBOL_GPL(locks_alloc_lock);
 
@@ -197,35 +209,12 @@ EXPORT_SYMBOL(locks_free_lock);
 
 void locks_init_lock(struct file_lock *fl)
 {
-	INIT_LIST_HEAD(&fl->fl_link);
-	INIT_LIST_HEAD(&fl->fl_block);
-	init_waitqueue_head(&fl->fl_wait);
-	fl->fl_next = NULL;
-	fl->fl_fasync = NULL;
-	fl->fl_owner = NULL;
-	fl->fl_pid = 0;
-	fl->fl_nspid = NULL;
-	fl->fl_file = NULL;
-	fl->fl_flags = 0;
-	fl->fl_type = 0;
-	fl->fl_start = fl->fl_end = 0;
-	fl->fl_ops = NULL;
-	fl->fl_lmops = NULL;
+	memset(fl, 0, sizeof(struct file_lock));
+	locks_init_lock_heads(fl);
 }
 
 EXPORT_SYMBOL(locks_init_lock);
 
-/*
- * Initialises the fields of the file lock which are invariant for
- * free file_locks.
- */
-static void init_once(void *foo)
-{
-	struct file_lock *lock = (struct file_lock *) foo;
-
-	locks_init_lock(lock);
-}
-
 static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
 {
 	if (fl->fl_ops) {
@@ -2323,8 +2312,8 @@ EXPORT_SYMBOL(lock_may_write);
 static int __init filelock_init(void)
 {
 	filelock_cache = kmem_cache_create("file_lock_cache",
-			sizeof(struct file_lock), 0, SLAB_PANIC,
-			init_once);
+			sizeof(struct file_lock), 0, SLAB_PANIC, NULL);
+
 	return 0;
 }
 

Re: [PATCH] fs: fix lock initialization

[Miklos Szeredi]

Miklos Szeredi <miklos@szeredi.hu> writes:

> "J. Bruce Fields" <bfields@fieldses.org> writes:
>
>> On Wed, Jul 06, 2011 at 12:33:55PM +0200, Miklos Szeredi wrote:
>>> From: Miklos Szeredi <mszeredi@suse.cz>
>>> 
>>> locks_alloc_lock() assumed that the allocated struct file_lock is
>>> already initialized to zero members.  This is only true for the first
>>> allocation of the structure, after reuse some of the members will have
>>> random values.
>>> 
>>> This will for example result in passing random fl_start values to
>>> userspace in fuse for FL_FLOCK locks, which is an information leak at
>>> best.
>>> 
>>> Fix by reinitializing those members which may be non-zero after freeing.
>>
>> Could you also just get rid of init_once() while you're at it?
>
> Sure.  Updated patch below.

Oh, the original was already applied.  Here's the incremental, if you
still want it.

Thanks,
Miklos
----


Subject: fs: locks: remove init_once

From: Miklos Szeredi <mszeredi@suse.cz>

Remove SLAB initialization entirely, as suggested by Bruce and Linus.
Allocate with __GFP_ZERO instead and only initialize list heads.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/locks.c |   41 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 31 deletions(-)

Index: linux-2.6/fs/locks.c
===================================================================
--- linux-2.6.orig/fs/locks.c	2011-07-07 13:03:49.000000000 +0200
+++ linux-2.6/fs/locks.c	2011-07-07 13:03:58.000000000 +0200
@@ -160,26 +160,20 @@ EXPORT_SYMBOL_GPL(unlock_flocks);
 
 static struct kmem_cache *filelock_cache __read_mostly;
 
-static void locks_init_lock_always(struct file_lock *fl)
+static void locks_init_lock_heads(struct file_lock *fl)
 {
-	fl->fl_next = NULL;
-	fl->fl_fasync = NULL;
-	fl->fl_owner = NULL;
-	fl->fl_pid = 0;
-	fl->fl_nspid = NULL;
-	fl->fl_file = NULL;
-	fl->fl_flags = 0;
-	fl->fl_type = 0;
-	fl->fl_start = fl->fl_end = 0;
+	INIT_LIST_HEAD(&fl->fl_link);
+	INIT_LIST_HEAD(&fl->fl_block);
+	init_waitqueue_head(&fl->fl_wait);
 }
 
 /* Allocate an empty lock structure. */
 struct file_lock *locks_alloc_lock(void)
 {
-	struct file_lock *fl = kmem_cache_alloc(filelock_cache, GFP_KERNEL);
+	struct file_lock *fl = kmem_cache_zalloc(filelock_cache, GFP_KERNEL);
 
 	if (fl)
-		locks_init_lock_always(fl);
+		locks_init_lock_heads(fl);
 
 	return fl;
 }
@@ -215,27 +209,12 @@ EXPORT_SYMBOL(locks_free_lock);
 
 void locks_init_lock(struct file_lock *fl)
 {
-	INIT_LIST_HEAD(&fl->fl_link);
-	INIT_LIST_HEAD(&fl->fl_block);
-	init_waitqueue_head(&fl->fl_wait);
-	fl->fl_ops = NULL;
-	fl->fl_lmops = NULL;
-	locks_init_lock_always(fl);
+	memset(fl, 0, sizeof(struct file_lock));
+	locks_init_lock_heads(fl);
 }
 
 EXPORT_SYMBOL(locks_init_lock);
 
-/*
- * Initialises the fields of the file lock which are invariant for
- * free file_locks.
- */
-static void init_once(void *foo)
-{
-	struct file_lock *lock = (struct file_lock *) foo;
-
-	locks_init_lock(lock);
-}
-
 static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
 {
 	if (fl->fl_ops) {
@@ -2333,8 +2312,8 @@ EXPORT_SYMBOL(lock_may_write);
 static int __init filelock_init(void)
 {
 	filelock_cache = kmem_cache_create("file_lock_cache",
-			sizeof(struct file_lock), 0, SLAB_PANIC,
-			init_once);
+			sizeof(struct file_lock), 0, SLAB_PANIC, NULL);
+
 	return 0;
 }
 

Re: [PATCH] fs: fix lock initialization

[Sebastian Pipping]

Hello!


Out of curiosity: which kernel release(s) is this likely to go into?
2.6.39.3?

Thanks,




Sebastian

Re: [PATCH] fs: fix lock initialization

[J. Bruce Fields]

On Thu, Jul 07, 2011 at 01:06:09PM +0200, Miklos Szeredi wrote:
> Miklos Szeredi <miklos@szeredi.hu> writes:
> 
> > "J. Bruce Fields" <bfields@fieldses.org> writes:
> >
> >> On Wed, Jul 06, 2011 at 12:33:55PM +0200, Miklos Szeredi wrote:
> >>> From: Miklos Szeredi <mszeredi@suse.cz>
> >>> 
> >>> locks_alloc_lock() assumed that the allocated struct file_lock is
> >>> already initialized to zero members.  This is only true for the first
> >>> allocation of the structure, after reuse some of the members will have
> >>> random values.
> >>> 
> >>> This will for example result in passing random fl_start values to
> >>> userspace in fuse for FL_FLOCK locks, which is an information leak at
> >>> best.
> >>> 
> >>> Fix by reinitializing those members which may be non-zero after freeing.
> >>
> >> Could you also just get rid of init_once() while you're at it?
> >
> > Sure.  Updated patch below.
> 
> Oh, the original was already applied.  Here's the incremental, if you
> still want it.

Sure, I'll queue it up with nfsd stuff if noone else takes it.

Is there any reason file locks should use a slab cache at all, actually?

The three different lock types (posix, flock, lease) don't each use all
the fields of "struct file_lock", and it might be clearer to reduce
struct file_lock to the minimal common fields and define posix locks,
flocks, and leases as separate structures embedding a file_lock.  (Well,
and then I suppose they could each get their own slab cache if someone
really though it mattered.)

--b.

> 
> Thanks,
> Miklos
> ----
> 
> 
> Subject: fs: locks: remove init_once
> 
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Remove SLAB initialization entirely, as suggested by Bruce and Linus.
> Allocate with __GFP_ZERO instead and only initialize list heads.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> ---
>  fs/locks.c |   41 ++++++++++-------------------------------
>  1 file changed, 10 insertions(+), 31 deletions(-)
> 
> Index: linux-2.6/fs/locks.c
> ===================================================================
> --- linux-2.6.orig/fs/locks.c	2011-07-07 13:03:49.000000000 +0200
> +++ linux-2.6/fs/locks.c	2011-07-07 13:03:58.000000000 +0200
> @@ -160,26 +160,20 @@ EXPORT_SYMBOL_GPL(unlock_flocks);
>  
>  static struct kmem_cache *filelock_cache __read_mostly;
>  
> -static void locks_init_lock_always(struct file_lock *fl)
> +static void locks_init_lock_heads(struct file_lock *fl)
>  {
> -	fl->fl_next = NULL;
> -	fl->fl_fasync = NULL;
> -	fl->fl_owner = NULL;
> -	fl->fl_pid = 0;
> -	fl->fl_nspid = NULL;
> -	fl->fl_file = NULL;
> -	fl->fl_flags = 0;
> -	fl->fl_type = 0;
> -	fl->fl_start = fl->fl_end = 0;
> +	INIT_LIST_HEAD(&fl->fl_link);
> +	INIT_LIST_HEAD(&fl->fl_block);
> +	init_waitqueue_head(&fl->fl_wait);
>  }
>  
>  /* Allocate an empty lock structure. */
>  struct file_lock *locks_alloc_lock(void)
>  {
> -	struct file_lock *fl = kmem_cache_alloc(filelock_cache, GFP_KERNEL);
> +	struct file_lock *fl = kmem_cache_zalloc(filelock_cache, GFP_KERNEL);
>  
>  	if (fl)
> -		locks_init_lock_always(fl);
> +		locks_init_lock_heads(fl);
>  
>  	return fl;
>  }
> @@ -215,27 +209,12 @@ EXPORT_SYMBOL(locks_free_lock);
>  
>  void locks_init_lock(struct file_lock *fl)
>  {
> -	INIT_LIST_HEAD(&fl->fl_link);
> -	INIT_LIST_HEAD(&fl->fl_block);
> -	init_waitqueue_head(&fl->fl_wait);
> -	fl->fl_ops = NULL;
> -	fl->fl_lmops = NULL;
> -	locks_init_lock_always(fl);
> +	memset(fl, 0, sizeof(struct file_lock));
> +	locks_init_lock_heads(fl);
>  }
>  
>  EXPORT_SYMBOL(locks_init_lock);
>  
> -/*
> - * Initialises the fields of the file lock which are invariant for
> - * free file_locks.
> - */
> -static void init_once(void *foo)
> -{
> -	struct file_lock *lock = (struct file_lock *) foo;
> -
> -	locks_init_lock(lock);
> -}
> -
>  static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>  {
>  	if (fl->fl_ops) {
> @@ -2333,8 +2312,8 @@ EXPORT_SYMBOL(lock_may_write);
>  static int __init filelock_init(void)
>  {
>  	filelock_cache = kmem_cache_create("file_lock_cache",
> -			sizeof(struct file_lock), 0, SLAB_PANIC,
> -			init_once);
> +			sizeof(struct file_lock), 0, SLAB_PANIC, NULL);
> +
>  	return 0;
>  }
>