linux-kernel - Re: [RESEND PATCH 05/13] vfs: Replace array of file pointers with an IDR

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] [day] [month] [year] [list]

Date:   Wed, 4 Oct 2017 17:45:05 +0200
From:   Mateusz Guzik <mguzik@...hat.com>
To:     Sandhya Bankar <bankarsandhya512@...il.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk, mawilcox@...rosoft.com,
        keescook@...omium.org, adobriyan@...il.com, re.emese@...il.com,
        riel@...riel.com
Subject: Re: [RESEND PATCH 05/13] vfs: Replace array of file pointers with an
 IDR

On Tue, Jul 11, 2017 at 06:50:52PM +0530, Sandhya Bankar wrote:
> Instead of storing all the file pointers in a single array, use an
> IDR.  It is RCU-safe, and does not need to be reallocated when the
> fd array grows.  It also handles allocation of new file descriptors.
> 
> ---
>  
[snip]
> @@ -604,22 +576,9 @@ void put_unused_fd(unsigned int fd)
>  void __fd_install(struct files_struct *files, unsigned int fd,
>  		struct file *file)
>  {
> -	struct fdtable *fdt;
> -
> -	might_sleep();
> -	rcu_read_lock_sched();
> -
> -	while (unlikely(files->resize_in_progress)) {
> -		rcu_read_unlock_sched();
> -		wait_event(files->resize_wait, !files->resize_in_progress);
> -		rcu_read_lock_sched();
> -	}
> -	/* coupled with smp_wmb() in expand_fdtable() */
> -	smp_rmb();
> -	fdt = rcu_dereference_sched(files->fdt);
> -	BUG_ON(fdt->fd[fd] != NULL);
> -	rcu_assign_pointer(fdt->fd[fd], file);
> -	rcu_read_unlock_sched();
> +	rcu_read_lock();
> +	BUG_ON(idr_replace(&files->fd_idr, file, fd));
> +	rcu_read_unlock();
>  }
>  
>  void fd_install(unsigned int fd, struct file *file)
> @@ -641,10 +600,9 @@ int __close_fd(struct files_struct *files, unsigned fd)
>  	fdt = files_fdtable(files);
>  	if (fd >= fdt->max_fds)
>  		goto out_unlock;
> -	file = fdt->fd[fd];
> +	file = idr_remove(&files->fd_idr, 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);

I have no opinions about the switch, however these 2 places make me
worried. I did not check all the changes so perhaps I missed something.

In the current code we are safe when it comes to concurrent install and
close, in particular here:

CPU0		CPU1
alloc_fd
		__close_fd
fd_install

__close_fd will either see a NULL pointer and return -EBADF or will see
an installed pointer and proceed with the close.

Your proposed patch seems to be buggy in this regard.

You call idr_remove, which from what I understand will free up the slot
no matter what. You only detect an error based on whether there was a
non-NULL pointer there or not. If so, fd_install can proceed to play
with a deallocated entry.

-- 
Mateusz Guzik