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
