linux-kernel - Re: [PATCH] Clean up expand_fdtable() and expand

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20060905095504.b76dca63.rdunlap@xenotime.net>
Date:	Tue, 5 Sep 2006 09:55:04 -0700
From:	"Randy.Dunlap" <rdunlap@...otime.net>
To:	Vadim Lobanov <vlobanov@...akeasy.net>
Cc:	akpm@...l.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Clean up expand_fdtable() and expand_files().

On Mon, 4 Sep 2006 22:08:36 -0700 Vadim Lobanov wrote:

> Hi,
> 
> This patch performs a code cleanup against the expand_fdtable() and 
> expand_files() functions inside fs/file.c. It aims to make the flow of code 
> within these functions simpler and easier to understand, via added comments 
> and modest refactoring. The patch was generated against 2.6.18-rc5-mm1, and 
> was successfully run live. Please apply.
> 
> (I'm trying out KMail for this patch. I tested this mailer beforehand to make 
> sure the patch will come out unmangled, but, as with all things software, 
> success is far from guaranteed. :) Please yell if the patch is borked.)

It's not (mechanically) b0rked.

> Signed-off-by: Vadim Lobanov <vlobanov@...akeasy.net>
> 
> diff -Npru linux-a/fs/file.c linux-b/fs/file.c
> --- linux-a/fs/file.c	2006-09-01 20:34:12.000000000 -0700
> +++ linux-b/fs/file.c	2006-09-04 16:42:33.000000000 -0700
> @@ -296,37 +296,30 @@ static int expand_fdtable(struct files_s
>  	__releases(files->file_lock)
>  	__acquires(files->file_lock)
>  {
> -	int error = 0;
> -	struct fdtable *fdt;
> -	struct fdtable *nfdt = NULL;
> +	struct fdtable *new_fdt, *cur_fdt;
>  
>  	spin_unlock(&files->file_lock);
> -	nfdt = alloc_fdtable(nr);
> -	if (!nfdt) {
> -		error = -ENOMEM;
> -		spin_lock(&files->file_lock);
> -		goto out;
> -	}
> -
> +	new_fdt = alloc_fdtable(nr);
>  	spin_lock(&files->file_lock);
> -	fdt = files_fdtable(files);
> +	if (!new_fdt)
> +		return -ENOMEM;
>  	/*
> -	 * Check again since another task may have expanded the
> -	 * fd table while we dropped the lock
> +	 * Check again since another task may have expanded the fd table while
> +	 * we dropped the lock
>  	 */
> -	if (nr >= fdt->max_fds || nr >= fdt->max_fdset) {
> -		copy_fdtable(nfdt, fdt);
> +	cur_fdt = files_fdtable(files);
> +	if (nr >= cur_fdt->max_fds || nr >= cur_fdt->max_fdset) {
> +		/* Continue as planned */
> +		copy_fdtable(new_fdt, cur_fdt);
> +		rcu_assign_pointer(files->fdt, new_fdt);
> +		free_fdtable(cur_fdt);
>  	} else {
> -		/* Somebody expanded while we dropped file_lock */
> +		/* Somebody else expanded, so undo our attempt */
>  		spin_unlock(&files->file_lock);
> -		__free_fdtable(nfdt);
> +		__free_fdtable(new_fdt);
>  		spin_lock(&files->file_lock);
> -		goto out;
>  	}
> -	rcu_assign_pointer(files->fdt, nfdt);
> -	free_fdtable(fdt);
> -out:
> -	return error;
> +	return 1;

This function didn't previously return a value of 1.
If it can do so now, please document it in the function comments
"header".  Using kernel-doc would be good too.

>  }
>  
>  /*
> @@ -336,23 +329,19 @@ out:
>   */
>  int expand_files(struct files_struct *files, int nr)
>  {
> -	int err, expand = 0;
>  	struct fdtable *fdt;
>  
>  	fdt = files_fdtable(files);
> -	if (nr >= fdt->max_fdset || nr >= fdt->max_fds) {
> -		if (fdt->max_fdset >= NR_OPEN ||
> -			fdt->max_fds >= NR_OPEN || nr >= NR_OPEN) {
> -			err = -EMFILE;
> -			goto out;
> -		}
> -		expand = 1;
> -		if ((err = expand_fdtable(files, nr)))
> -			goto out;
> -	}
> -	err = expand;
> -out:
> -	return err;
> +	/* Do we need to expand? */
> +	if (nr < fdt->max_fdset && nr < fdt->max_fds)
> +		return 0;
> +	/* Can we expand? */
> +	if (fdt->max_fdset >= NR_OPEN || fdt->max_fds >= NR_OPEN ||
> +	    nr >= NR_OPEN)
> +		return -EMFILE;
> +
> +	/* All good, so we try */
> +	return expand_fdtable(files, nr);
>  }
>  
>  static void __devinit fdtable_defer_list_init(int cpu)


---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/