On Fri, Dec 08, 2023 at 01:47:35PM +0800, Baolu Lu wrote: > On 12/8/23 1:17 AM, Jason Gunthorpe wrote: > > On Thu, Dec 07, 2023 at 05:34:10PM +0100, Joel Granados wrote: > > > > @@ -58,6 +255,8 @@ static void hw_pagetable_fault_free(struct hw_pgtable_fault *fault) > > > > WARN_ON(!list_empty(&fault->deliver)); > > > > WARN_ON(!list_empty(&fault->response)); > > > > + fput(fault->fault_file); > > > > + put_unused_fd(fault->fault_fd); > > > I have resolved this in a naive way by just not calling the > > > put_unused_fd function. > > That is correct. > > > > put_unused_fd() should only be called on error paths prior to the > > syscall return. > > > > The design of a FD must follow this pattern > > > > syscall(): > > fdno = get_unused_fd_flags(O_CLOEXEC); > > filep = [..] > > // syscall MUST succeed after this statement: > > fd_install(fdno, filep); > > return 0; > > > > err: > > put_unused_fd(fdno) > > return -ERRNO > > Yes. Agreed. > > > > > Also the refcounting looks a little strange, the filep reference is > > consumed by fd_install, so what is that fput pairing with in fault_free? > > fput() pairs with get_unused_fd_flags()? fd_install() does not seem to > increase any reference. fd_install() transfers the reference to the fd table and that reference is put back by the close() system call. Notice that instantly after fd_install() a concurrent user can free the filep. Jason
