On Thu, Jul 07, 2011 at 01:06:09PM +0200, Miklos Szeredi wrote:
> Miklos Szeredi <miklos@xxxxxxxxxx> writes:
>
> > "J. Bruce Fields" <bfields@xxxxxxxxxxxx> writes:
> >
> >> On Wed, Jul 06, 2011 at 12:33:55PM +0200, Miklos Szeredi wrote:
> >>> From: Miklos Szeredi <mszeredi@xxxxxxx>
> >>>
> >>> 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@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
