Re: [PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Mateusz Guzik]

On Wed, Dec 11, 2013 at 10:08:27PM +0100, Yann Droneaud wrote:
> close-on-exec flag is set by get_unused_fd_flags(), it makes
> sense to clear it in the opposite function, eg. put_unused_fd().
> 
> Additionally, since the close_on_exec bit array is always initialized
> to 0, it can be safely assumed that any newly allocated file descriptor
> has close-on-exec flag not set, so there's no need to clear it
> explicitly.
> 
> Signed-off-by: Yann Droneaud <ydroneaud@xxxxxxxxxx>
> ---
>  fs/file.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 4a78f981557a..e98f5a5b1050 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -500,8 +500,6 @@ repeat:
>  	__set_open_fd(fd, fdt);
>  	if (flags & O_CLOEXEC)
>  		__set_close_on_exec(fd, fdt);
> -	else
> -		__clear_close_on_exec(fd, fdt);
>  	error = fd;
>  #if 1
>  	/* Sanity check */
> @@ -530,6 +528,7 @@ EXPORT_SYMBOL(get_unused_fd_flags);
>  static void __put_unused_fd(struct files_struct *files, unsigned int fd)
>  {
>  	struct fdtable *fdt = files_fdtable(files);
> +	__clear_close_on_exec(fd, fdt);
>  	__clear_open_fd(fd, fdt);
>  	if (fd < files->next_fd)
>  		files->next_fd = fd;
> @@ -599,7 +598,6 @@ int __close_fd(struct files_struct *files, unsigned fd)
>  	if (!file)
>  		goto out_unlock;
>  	rcu_assign_pointer(fdt->fd[fd], NULL);
> -	__clear_close_on_exec(fd, fdt);
>  	__put_unused_fd(files, fd);
>  	spin_unlock(&files->file_lock);
>  	return filp_close(file, files);
> @@ -625,7 +623,6 @@ void do_close_on_exec(struct files_struct *files)
>  		set = fdt->close_on_exec[i];
>  		if (!set)
>  			continue;
> -		fdt->close_on_exec[i] = 0;
>  		for ( ; set ; fd++, set >>= 1) {
>  			struct file *file;
>  			if (!(set & 1))
> @@ -806,8 +803,6 @@ static int do_dup2(struct files_struct *files,
>  	__set_open_fd(fd, fdt);
>  	if (flags & O_CLOEXEC)
>  		__set_close_on_exec(fd, fdt);
> -	else
> -		__clear_close_on_exec(fd, fdt);
>  	spin_unlock(&files->file_lock);
>  
>  	if (tofree)

>From my reading this will break at least the following:
fd = open(..., .. | O_CLOEXEC);
dup2(whatever, fd);

now fd has O_CLOEXEC even though it should not

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