* [PATCH] CIFS: fix circular locking dependency
@ 2017-06-29 14:01 Rabin Vincent
2017-07-06 0:59 ` Pavel Shilovskiy
2017-07-06 1:03 ` Steve French
0 siblings, 2 replies; 3+ messages in thread
From: Rabin Vincent @ 2017-06-29 14:01 UTC (permalink / raw)
To: sfrench; +Cc: linux-cifs, linux-kernel, viro, Rabin Vincent
From: Rabin Vincent <rabinv@axis.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@axis.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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH] CIFS: fix circular locking dependency
2017-06-29 14:01 [PATCH] CIFS: fix circular locking dependency Rabin Vincent
@ 2017-07-06 0:59 ` Pavel Shilovskiy
2017-07-06 1:03 ` Steve French
1 sibling, 0 replies; 3+ messages in thread
From: Pavel Shilovskiy @ 2017-07-06 0:59 UTC (permalink / raw)
To: Rabin Vincent, sfrench; +Cc: linux-cifs, linux-kernel, viro, Rabin Vincent
2017-06-29 7:01 GMT-07:00 Rabin Vincent <rabin.vincent@axis.com>:
> From: Rabin Vincent <rabinv@axis.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@axis.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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] CIFS: fix circular locking dependency
2017-06-29 14:01 [PATCH] CIFS: fix circular locking dependency Rabin Vincent
2017-07-06 0:59 ` Pavel Shilovskiy
@ 2017-07-06 1:03 ` Steve French
1 sibling, 0 replies; 3+ messages in thread
From: Steve French @ 2017-07-06 1:03 UTC (permalink / raw)
To: Rabin Vincent; +Cc: Steve French, linux-cifs, LKML, Al Viro, Rabin Vincent
merged into cifs-2.6.git for-next
On Thu, Jun 29, 2017 at 9:01 AM, Rabin Vincent <rabin.vincent@axis.com> wrote:
> From: Rabin Vincent <rabinv@axis.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@axis.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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-06 1:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 14:01 [PATCH] CIFS: fix circular locking dependency Rabin Vincent
2017-07-06 0:59 ` Pavel Shilovskiy
2017-07-06 1:03 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).