From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752618AbdJDPpM (ORCPT ); Wed, 4 Oct 2017 11:45:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63244 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752409AbdJDPpK (ORCPT ); Wed, 4 Oct 2017 11:45:10 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3D01817FD Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=mguzik@redhat.com Date: Wed, 4 Oct 2017 17:45:05 +0200 From: Mateusz Guzik To: Sandhya Bankar Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk, mawilcox@microsoft.com, keescook@chromium.org, adobriyan@gmail.com, re.emese@gmail.com, riel@surriel.com Subject: Re: [RESEND PATCH 05/13] vfs: Replace array of file pointers with an IDR Message-ID: <20171004154504.drdlu7zavtk3nk27@mguzik> References: <3f984d2111ae4856bf5183173b0f9fcaaefed8c0.1493315290.git.bankarsandhya512@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <3f984d2111ae4856bf5183173b0f9fcaaefed8c0.1493315290.git.bankarsandhya512@gmail.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 04 Oct 2017 15:45:10 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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