* [PATCH] ufs: fix deadlocks after mutex merge
@ 2014-09-01 22:30 Alexey Khoroshilov
2014-09-01 23:08 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Khoroshilov @ 2014-09-01 22:30 UTC (permalink / raw)
To: Evgeniy Dushistov
Cc: Alexey Khoroshilov, Andrew Morton, Linus Torvalds, linux-kernel,
ldv-project
Commit 0244756edc4b ("ufs: sb mutex merge + mutex_destroy") introduces
deadlocks in ufs_new_inode() and ufs_free_inode() that call lock_ufs()
being already invoked with mutex held.
ufs_free_inode() is always invoked with mutex locked, while
ufs_new_inode() is called with mutex locked two times of four.
The patch proposes to resolve the issue by agreement to call
ufs_new_inode() and ufs_free_inode() with mutex unheld.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
fs/ufs/inode.c | 7 ++-----
fs/ufs/namei.c | 14 ++++++--------
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 7c580c97990e..be7d42c7d938 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -902,9 +902,6 @@ void ufs_evict_inode(struct inode * inode)
invalidate_inode_buffers(inode);
clear_inode(inode);
- if (want_delete) {
- lock_ufs(inode->i_sb);
- ufs_free_inode (inode);
- unlock_ufs(inode->i_sb);
- }
+ if (want_delete)
+ ufs_free_inode(inode);
}
diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c
index 90d74b8f8eba..2df62a73f20c 100644
--- a/fs/ufs/namei.c
+++ b/fs/ufs/namei.c
@@ -126,12 +126,12 @@ static int ufs_symlink (struct inode * dir, struct dentry * dentry,
if (l > sb->s_blocksize)
goto out_notlocked;
- lock_ufs(dir->i_sb);
inode = ufs_new_inode(dir, S_IFLNK | S_IRWXUGO);
err = PTR_ERR(inode);
if (IS_ERR(inode))
- goto out;
+ goto out_notlocked;
+ lock_ufs(dir->i_sb);
if (l > UFS_SB(sb)->s_uspi->s_maxsymlinklen) {
/* slow symlink */
inode->i_op = &ufs_symlink_inode_operations;
@@ -181,13 +181,9 @@ static int ufs_mkdir(struct inode * dir, struct dentry * dentry, umode_t mode)
struct inode * inode;
int err;
- lock_ufs(dir->i_sb);
- inode_inc_link_count(dir);
-
inode = ufs_new_inode(dir, S_IFDIR|mode);
- err = PTR_ERR(inode);
if (IS_ERR(inode))
- goto out_dir;
+ return PTR_ERR(inode);
inode->i_op = &ufs_dir_inode_operations;
inode->i_fop = &ufs_dir_operations;
@@ -195,6 +191,9 @@ static int ufs_mkdir(struct inode * dir, struct dentry * dentry, umode_t mode)
inode_inc_link_count(inode);
+ lock_ufs(dir->i_sb);
+ inode_inc_link_count(dir);
+
err = ufs_make_empty(inode, dir);
if (err)
goto out_fail;
@@ -212,7 +211,6 @@ out_fail:
inode_dec_link_count(inode);
inode_dec_link_count(inode);
iput (inode);
-out_dir:
inode_dec_link_count(dir);
unlock_ufs(dir->i_sb);
goto out;
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ufs: fix deadlocks after mutex merge
2014-09-01 22:30 [PATCH] ufs: fix deadlocks after mutex merge Alexey Khoroshilov
@ 2014-09-01 23:08 ` Al Viro
2014-09-01 23:13 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: Al Viro @ 2014-09-01 23:08 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: Evgeniy Dushistov, Andrew Morton, Linus Torvalds, linux-kernel,
ldv-project
On Tue, Sep 02, 2014 at 02:30:24AM +0400, Alexey Khoroshilov wrote:
> Commit 0244756edc4b ("ufs: sb mutex merge + mutex_destroy") introduces
> deadlocks in ufs_new_inode() and ufs_free_inode() that call lock_ufs()
> being already invoked with mutex held.
>
> ufs_free_inode() is always invoked with mutex locked, while
> ufs_new_inode() is called with mutex locked two times of four.
>
> The patch proposes to resolve the issue by agreement to call
> ufs_new_inode() and ufs_free_inode() with mutex unheld.
> @@ -902,9 +902,6 @@ void ufs_evict_inode(struct inode * inode)
> invalidate_inode_buffers(inode);
> clear_inode(inode);
>
> - if (want_delete) {
> - lock_ufs(inode->i_sb);
> - ufs_free_inode (inode);
> - unlock_ufs(inode->i_sb);
> - }
> + if (want_delete)
> + ufs_free_inode(inode);
Your commit message makes no sense - ufs_evict_inode() is *never* called
with that lock held, for one thing. I agree that "ufs: sb mutex merge +
mutex_destroy" was been badly broken and apparently never tested, though -
the bugs are real.
Please, write a saner commit message; what happens is that
ufs_{new,free}_inode() take the damn lock themselves these days, so
their caller shouldn't do that.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ufs: fix deadlocks after mutex merge
2014-09-01 23:08 ` Al Viro
@ 2014-09-01 23:13 ` Al Viro
2014-09-02 7:40 ` [PATCH v2] ufs: fix deadlocks introduced by sb " Alexey Khoroshilov
0 siblings, 1 reply; 4+ messages in thread
From: Al Viro @ 2014-09-01 23:13 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: Evgeniy Dushistov, Andrew Morton, Linus Torvalds, linux-kernel,
ldv-project
On Tue, Sep 02, 2014 at 12:08:35AM +0100, Al Viro wrote:
> Your commit message makes no sense - ufs_evict_inode() is *never* called
> with that lock held, for one thing. I agree that "ufs: sb mutex merge +
> mutex_destroy" was been badly broken and apparently never tested, though -
> the bugs are real.
>
> Please, write a saner commit message; what happens is that
> ufs_{new,free}_inode() take the damn lock themselves these days, so
> their caller shouldn't do that.
PS: after rereading your mail, I see what you meant to say, but it really
wasn't clear enough. The crucial part is that the lock is *always* taken
by free/new, not that some of the callers take it and some do not.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] ufs: fix deadlocks introduced by sb mutex merge
2014-09-01 23:13 ` Al Viro
@ 2014-09-02 7:40 ` Alexey Khoroshilov
0 siblings, 0 replies; 4+ messages in thread
From: Alexey Khoroshilov @ 2014-09-02 7:40 UTC (permalink / raw)
To: Al Viro
Cc: Alexey Khoroshilov, Evgeniy Dushistov, Andrew Morton,
Linus Torvalds, linux-kernel, ldv-project
Commit 0244756edc4b ("ufs: sb mutex merge + mutex_destroy") introduces
deadlocks in ufs_new_inode() and ufs_free_inode().
Most callers of that functions acqure the mutex by themselves and
ufs_{new,free}_inode() do that via lock_ufs(),
i.e we have an unavoidable double lock.
The patch proposes to resolve the issue by making sure that
ufs_{new,free}_inode() are not called with the mutex held.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
fs/ufs/inode.c | 7 ++-----
fs/ufs/namei.c | 14 ++++++--------
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 7c580c97990e..be7d42c7d938 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -902,9 +902,6 @@ void ufs_evict_inode(struct inode * inode)
invalidate_inode_buffers(inode);
clear_inode(inode);
- if (want_delete) {
- lock_ufs(inode->i_sb);
- ufs_free_inode (inode);
- unlock_ufs(inode->i_sb);
- }
+ if (want_delete)
+ ufs_free_inode(inode);
}
diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c
index 90d74b8f8eba..2df62a73f20c 100644
--- a/fs/ufs/namei.c
+++ b/fs/ufs/namei.c
@@ -126,12 +126,12 @@ static int ufs_symlink (struct inode * dir, struct dentry * dentry,
if (l > sb->s_blocksize)
goto out_notlocked;
- lock_ufs(dir->i_sb);
inode = ufs_new_inode(dir, S_IFLNK | S_IRWXUGO);
err = PTR_ERR(inode);
if (IS_ERR(inode))
- goto out;
+ goto out_notlocked;
+ lock_ufs(dir->i_sb);
if (l > UFS_SB(sb)->s_uspi->s_maxsymlinklen) {
/* slow symlink */
inode->i_op = &ufs_symlink_inode_operations;
@@ -181,13 +181,9 @@ static int ufs_mkdir(struct inode * dir, struct dentry * dentry, umode_t mode)
struct inode * inode;
int err;
- lock_ufs(dir->i_sb);
- inode_inc_link_count(dir);
-
inode = ufs_new_inode(dir, S_IFDIR|mode);
- err = PTR_ERR(inode);
if (IS_ERR(inode))
- goto out_dir;
+ return PTR_ERR(inode);
inode->i_op = &ufs_dir_inode_operations;
inode->i_fop = &ufs_dir_operations;
@@ -195,6 +191,9 @@ static int ufs_mkdir(struct inode * dir, struct dentry * dentry, umode_t mode)
inode_inc_link_count(inode);
+ lock_ufs(dir->i_sb);
+ inode_inc_link_count(dir);
+
err = ufs_make_empty(inode, dir);
if (err)
goto out_fail;
@@ -212,7 +211,6 @@ out_fail:
inode_dec_link_count(inode);
inode_dec_link_count(inode);
iput (inode);
-out_dir:
inode_dec_link_count(dir);
unlock_ufs(dir->i_sb);
goto out;
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-02 7:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-01 22:30 [PATCH] ufs: fix deadlocks after mutex merge Alexey Khoroshilov
2014-09-01 23:08 ` Al Viro
2014-09-01 23:13 ` Al Viro
2014-09-02 7:40 ` [PATCH v2] ufs: fix deadlocks introduced by sb " Alexey Khoroshilov
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).