linux-kernel - Re: [PATCH] CIFS: fix circular locking dependency

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]
Message-ID: <CAH2r5muzJv2x89kYA4E-j7UDd=iHAy8EbDK4Oi5QFKt3STGqVg@mail.gmail.com>
Date:   Wed, 5 Jul 2017 20:03:23 -0500
From:   Steve French <smfrench@...il.com>
To:     Rabin Vincent <rabin.vincent@...s.com>
Cc:     Steve French <sfrench@...ba.org>,
        "linux-cifs@...r.kernel.org" <linux-cifs@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Rabin Vincent <rabinv@...s.com>
Subject: Re: [PATCH] CIFS: fix circular locking dependency

merged into cifs-2.6.git for-next

On Thu, Jun 29, 2017 at 9:01 AM, Rabin Vincent <rabin.vincent@...s.com> wrote:
> From: Rabin Vincent <rabinv@...s.com>
>
> When a CIFS filesystem is mounted with the forcemand option and the
> following command is run on it, lockdep warns about a circular locking
> dependency between CifsInodeInfo::lock_sem and the inode lock.
>
>  while echo foo > hello; do :; done & while touch -c hello; do :; done
>
> cifs_writev() takes the locks in the wrong order, but note that we can't
> only flip the order around because it releases the inode lock before the
> call to generic_write_sync() while it holds the lock_sem across that
> call.
>
> But, AFAICS, there is no need to hold the CifsInodeInfo::lock_sem across
> the generic_write_sync() call either, so we can release both the locks
> before generic_write_sync(), and change the order.
>
>  ======================================================
>  WARNING: possible circular locking dependency detected
>  4.12.0-rc7+ #9 Not tainted
>  ------------------------------------------------------
>  touch/487 is trying to acquire lock:
>   (&cifsi->lock_sem){++++..}, at: cifsFileInfo_put+0x88f/0x16a0
>
>  but task is already holding lock:
>   (&sb->s_type->i_mutex_key#11){+.+.+.}, at: utimes_common+0x3ad/0x870
>
>  which lock already depends on the new lock.
>
>  the existing dependency chain (in reverse order) is:
>
>  -> #1 (&sb->s_type->i_mutex_key#11){+.+.+.}:
>         __lock_acquire+0x1f74/0x38f0
>         lock_acquire+0x1cc/0x600
>         down_write+0x74/0x110
>         cifs_strict_writev+0x3cb/0x8c0
>         __vfs_write+0x4c1/0x930
>         vfs_write+0x14c/0x2d0
>         SyS_write+0xf7/0x240
>         entry_SYSCALL_64_fastpath+0x1f/0xbe
>
>  -> #0 (&cifsi->lock_sem){++++..}:
>         check_prevs_add+0xfa0/0x1d10
>         __lock_acquire+0x1f74/0x38f0
>         lock_acquire+0x1cc/0x600
>         down_write+0x74/0x110
>         cifsFileInfo_put+0x88f/0x16a0
>         cifs_setattr+0x992/0x1680
>         notify_change+0x61a/0xa80
>         utimes_common+0x3d4/0x870
>         do_utimes+0x1c1/0x220
>         SyS_utimensat+0x84/0x1a0
>         entry_SYSCALL_64_fastpath+0x1f/0xbe
>
>  other info that might help us debug this:
>
>   Possible unsafe locking scenario:
>
>         CPU0                    CPU1
>         ----                    ----
>    lock(&sb->s_type->i_mutex_key#11);
>                                 lock(&cifsi->lock_sem);
>                                 lock(&sb->s_type->i_mutex_key#11);
>    lock(&cifsi->lock_sem);
>
>   *** DEADLOCK ***
>
>  2 locks held by touch/487:
>   #0:  (sb_writers#10){.+.+.+}, at: mnt_want_write+0x41/0xb0
>   #1:  (&sb->s_type->i_mutex_key#11){+.+.+.}, at: utimes_common+0x3ad/0x870
>
>  stack backtrace:
>  CPU: 0 PID: 487 Comm: touch Not tainted 4.12.0-rc7+ #9
>  Call Trace:
>   dump_stack+0xdb/0x185
>   print_circular_bug+0x45b/0x790
>   __lock_acquire+0x1f74/0x38f0
>   lock_acquire+0x1cc/0x600
>   down_write+0x74/0x110
>   cifsFileInfo_put+0x88f/0x16a0
>   cifs_setattr+0x992/0x1680
>   notify_change+0x61a/0xa80
>   utimes_common+0x3d4/0x870
>   do_utimes+0x1c1/0x220
>   SyS_utimensat+0x84/0x1a0
>   entry_SYSCALL_64_fastpath+0x1f/0xbe
>
> Fixes: 19dfc1f5f2ef03a52 ("cifs: fix the race in cifs_writev()")
> Signed-off-by: Rabin Vincent <rabinv@...s.com>
> ---
>  fs/cifs/file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index fcef706..d16fa55 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2810,12 +2810,12 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>         struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
>         ssize_t rc;
>
> +       inode_lock(inode);
>         /*
>          * We need to hold the sem to be sure nobody modifies lock list
>          * with a brlock that prevents writing.
>          */
>         down_read(&cinode->lock_sem);
> -       inode_lock(inode);
>
>         rc = generic_write_checks(iocb, from);
>         if (rc <= 0)
> @@ -2828,11 +2828,11 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>         else
>                 rc = -EACCES;
>  out:
> +       up_read(&cinode->lock_sem);
>         inode_unlock(inode);
>
>         if (rc > 0)
>                 rc = generic_write_sync(iocb, rc);
> -       up_read(&cinode->lock_sem);
>         return rc;
>  }
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve