• David Howells's avatar
    vfs: Fix potential circular locking through setxattr() and removexattr() · c3a5e3e8
    David Howells authored
    When using cachefiles, lockdep may emit something similar to the circular
    locking dependency notice below.  The problem appears to stem from the
    following:
    
     (1) Cachefiles manipulates xattrs on the files in its cache when called
         from ->writepages().
    
     (2) The setxattr() and removexattr() system call handlers get the name
         (and value) from userspace after taking the sb_writers lock, putting
         accesses of the vma->vm_lock and mm->mmap_lock inside of that.
    
     (3) The afs filesystem uses a per-inode lock to prevent multiple
         revalidation RPCs and in writeback vs truncate to prevent parallel
         operations from deadlocking against the server on one side and local
         page locks on the other.
    
    Fix this by moving the getting of the name and value in {get,remove}xattr()
    outside of the sb_writers lock.  This also has the minor benefits that we
    don't need to reget these in the event of a retry and we never try to take
    the sb_writers lock in the event we can't pull the name and value into the
    kernel.
    
    Alternative approaches that might fix this include moving the dispatch of a
    write to the cache off to a workqueue or trying to do without the
    validation lock in afs.  Note that this might also affect other filesystems
    that use netfslib and/or cachefiles.
    
     ======================================================
     WARNING: possible circular locking dependency detected
     6.10.0-build2+ #956 Not tainted
     ------------------------------------------------------
     fsstress/6050 is trying to acquire lock:
     ffff888138fd82f0 (mapping.invalidate_lock#3){++++}-{3:3}, at: filemap_fault+0x26e/0x8b0
    
     but task is already holding lock:
     ffff888113f26d18 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x165/0x250
    
     which lock already depends on the new lock.
    
     the existing dependency chain (in reverse order) is:
    
     -> #4 (&vma->vm_lock->lock){++++}-{3:3}:
            __lock_acquire+0xaf0/0xd80
            lock_acquire.part.0+0x103/0x280
            down_write+0x3b/0x50
            vma_start_write+0x6b/0xa0
            vma_link+0xcc/0x140
            insert_vm_struct+0xb7/0xf0
            alloc_bprm+0x2c1/0x390
            kernel_execve+0x65/0x1a0
            call_usermodehelper_exec_async+0x14d/0x190
            ret_from_fork+0x24/0x40
            ret_from_fork_asm+0x1a/0x30
    
     -> #3 (&mm->mmap_lock){++++}-{3:3}:
            __lock_acquire+0xaf0/0xd80
            lock_acquire.part.0+0x103/0x280
            __might_fault+0x7c/0xb0
            strncpy_from_user+0x25/0x160
            removexattr+0x7f/0x100
            __do_sys_fremovexattr+0x7e/0xb0
            do_syscall_64+0x9f/0x100
            entry_SYSCALL_64_after_hwframe+0x76/0x7e
    
     -> #2 (sb_writers#14){.+.+}-{0:0}:
            __lock_acquire+0xaf0/0xd80
            lock_acquire.part.0+0x103/0x280
            percpu_down_read+0x3c/0x90
            vfs_iocb_iter_write+0xe9/0x1d0
            __cachefiles_write+0x367/0x430
            cachefiles_issue_write+0x299/0x2f0
            netfs_advance_write+0x117/0x140
            netfs_write_folio.isra.0+0x5ca/0x6e0
            netfs_writepages+0x230/0x2f0
            afs_writepages+0x4d/0x70
            do_writepages+0x1e8/0x3e0
            filemap_fdatawrite_wbc+0x84/0xa0
            __filemap_fdatawrite_range+0xa8/0xf0
            file_write_and_wait_range+0x59/0x90
            afs_release+0x10f/0x270
            __fput+0x25f/0x3d0
            __do_sys_close+0x43/0x70
            do_syscall_64+0x9f/0x100
            entry_SYSCALL_64_after_hwframe+0x76/0x7e
    
     -> #1 (&vnode->validate_lock){++++}-{3:3}:
            __lock_acquire+0xaf0/0xd80
            lock_acquire.part.0+0x103/0x280
            down_read+0x95/0x200
            afs_writepages+0x37/0x70
            do_writepages+0x1e8/0x3e0
            filemap_fdatawrite_wbc+0x84/0xa0
            filemap_invalidate_inode+0x167/0x1e0
            netfs_unbuffered_write_iter+0x1bd/0x2d0
            vfs_write+0x22e/0x320
            ksys_write+0xbc/0x130
            do_syscall_64+0x9f/0x100
            entry_SYSCALL_64_after_hwframe+0x76/0x7e
    
     -> #0 (mapping.invalidate_lock#3){++++}-{3:3}:
            check_noncircular+0x119/0x160
            check_prev_add+0x195/0x430
            __lock_acquire+0xaf0/0xd80
            lock_acquire.part.0+0x103/0x280
            down_read+0x95/0x200
            filemap_fault+0x26e/0x8b0
            __do_fault+0x57/0xd0
            do_pte_missing+0x23b/0x320
            __handle_mm_fault+0x2d4/0x320
            handle_mm_fault+0x14f/0x260
            do_user_addr_fault+0x2a2/0x500
            exc_page_fault+0x71/0x90
            asm_exc_page_fault+0x22/0x30
    
     other info that might help us debug this:
    
     Chain exists of:
       mapping.invalidate_lock#3 --> &mm->mmap_lock --> &vma->vm_lock->lock
    
      Possible unsafe locking scenario:
    
            CPU0                    CPU1
            ----                    ----
       rlock(&vma->vm_lock->lock);
                                    lock(&mm->mmap_lock);
                                    lock(&vma->vm_lock->lock);
       rlock(mapping.invalidate_lock#3);
    
      *** DEADLOCK ***
    
     1 lock held by fsstress/6050:
      #0: ffff888113f26d18 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x165/0x250
    
     stack backtrace:
     CPU: 0 PID: 6050 Comm: fsstress Not tainted 6.10.0-build2+ #956
     Hardware name: ASUS All Series/H97-PLUS, BIOS 2306 10/09/2014
     Call Trace:
      <TASK>
      dump_stack_lvl+0x57/0x80
      check_noncircular+0x119/0x160
      ? queued_spin_lock_slowpath+0x4be/0x510
      ? __pfx_check_noncircular+0x10/0x10
      ? __pfx_queued_spin_lock_slowpath+0x10/0x10
      ? mark_lock+0x47/0x160
      ? init_chain_block+0x9c/0xc0
      ? add_chain_block+0x84/0xf0
      check_prev_add+0x195/0x430
      __lock_acquire+0xaf0/0xd80
      ? __pfx___lock_acquire+0x10/0x10
      ? __lock_release.isra.0+0x13b/0x230
      lock_acquire.part.0+0x103/0x280
      ? filemap_fault+0x26e/0x8b0
      ? __pfx_lock_acquire.part.0+0x10/0x10
      ? rcu_is_watching+0x34/0x60
      ? lock_acquire+0xd7/0x120
      down_read+0x95/0x200
      ? filemap_fault+0x26e/0x8b0
      ? __pfx_down_read+0x10/0x10
      ? __filemap_get_folio+0x25/0x1a0
      filemap_fault+0x26e/0x8b0
      ? __pfx_filemap_fault+0x10/0x10
      ? find_held_lock+0x7c/0x90
      ? __pfx___lock_release.isra.0+0x10/0x10
      ? __pte_offset_map+0x99/0x110
      __do_fault+0x57/0xd0
      do_pte_missing+0x23b/0x320
      __handle_mm_fault+0x2d4/0x320
      ? __pfx___handle_mm_fault+0x10/0x10
      handle_mm_fault+0x14f/0x260
      do_user_addr_fault+0x2a2/0x500
      exc_page_fault+0x71/0x90
      asm_exc_page_fault+0x22/0x30
    Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
    Link: https://lore.kernel.org/r/2136178.1721725194@warthog.procyon.org.uk
    cc: Alexander Viro <viro@zeniv.linux.org.uk>
    cc: Christian Brauner <brauner@kernel.org>
    cc: Jan Kara <jack@suse.cz>
    cc: Jeff Layton <jlayton@kernel.org>
    cc: Gao Xiang <xiang@kernel.org>
    cc: Matthew Wilcox <willy@infradead.org>
    cc: netfs@lists.linux.dev
    cc: linux-erofs@lists.ozlabs.org
    cc: linux-fsdevel@vger.kernel.org
    [brauner: fix minor issues]
    Signed-off-by: default avatarChristian Brauner <brauner@kernel.org>
    c3a5e3e8
xattr.c 36.1 KB