* [PATCH] ocfs2: update d_splice_alias() return code checking
@ 2025-06-26 2:14 Tetsuo Handa
2025-06-26 3:34 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2025-06-26 2:14 UTC (permalink / raw)
To: Mark Fasheh, Joel Becker, Joseph Qi, Richard Weinberger, Al Viro,
ocfs2-devel, linux-fsdevel, LKML, Andrew Morton
When commit d3556babd7fa ("ocfs2: fix d_splice_alias() return code
checking") was merged into v3.18-rc3, d_splice_alias() was returning
one of a valid dentry, NULL or an ERR_PTR.
But when commit b5ae6b15bd73 ("merge d_materialise_unique() into
d_splice_alias()") was merged into v3.19-rc1, d_splice_alias() started
returning -ELOOP as one of ERR_PTR values.
As a result, when syzkaller mounts a crafted ocfs2 filesystem image that
hits d_splice_alias() == -ELOOP case from ocfs2_lookup(), ocfs2_lookup()
fails to handle -ELOOP case and generic_shutdown_super() hits "VFS: Busy
inodes after unmount" message.
Don't call ocfs2_dentry_attach_lock() nor ocfs2_dentry_attach_gen()
when d_splice_alias() returned -ELOOP.
Reported-by: syzbot <syzbot+1134d3a5b062e9665a7a@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=1134d3a5b062e9665a7a
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
This patch wants review from maintainers. I'm not familiar with this change.
fs/ocfs2/namei.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 99278c8f0e24..4ccb39f43bc6 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -142,6 +142,8 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
bail_add:
ret = d_splice_alias(inode, dentry);
+ if (ret == ERR_PTR(-ELOOP))
+ goto bail_unlock;
if (inode) {
/*
--
2.49.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ocfs2: update d_splice_alias() return code checking
2025-06-26 2:14 [PATCH] ocfs2: update d_splice_alias() return code checking Tetsuo Handa
@ 2025-06-26 3:34 ` Al Viro
2025-06-27 14:19 ` [PATCH v2] " Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2025-06-26 3:34 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Mark Fasheh, Joel Becker, Joseph Qi, Richard Weinberger,
ocfs2-devel, linux-fsdevel, LKML, Andrew Morton
On Thu, Jun 26, 2025 at 11:14:59AM +0900, Tetsuo Handa wrote:
> But when commit b5ae6b15bd73 ("merge d_materialise_unique() into
> d_splice_alias()") was merged into v3.19-rc1, d_splice_alias() started
> returning -ELOOP as one of ERR_PTR values.
>
> As a result, when syzkaller mounts a crafted ocfs2 filesystem image that
> hits d_splice_alias() == -ELOOP case from ocfs2_lookup(), ocfs2_lookup()
> fails to handle -ELOOP case and generic_shutdown_super() hits "VFS: Busy
> inodes after unmount" message.
>
> Don't call ocfs2_dentry_attach_lock() nor ocfs2_dentry_attach_gen()
> when d_splice_alias() returned -ELOOP.
>
> Reported-by: syzbot <syzbot+1134d3a5b062e9665a7a@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=1134d3a5b062e9665a7a
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> This patch wants review from maintainers. I'm not familiar with this change.
Not the right fix. If nothing else, -ELOOP is not the only possible value
there.
This
status = ocfs2_dentry_attach_lock(dentry, inode,
OCFS2_I(dir)->ip_blkno);
if (status) {
mlog_errno(status);
ret = ERR_PTR(status);
goto bail_unlock;
}
looks like pretty obvious leak in its own right? What's more, on IS_ERR(ret)
we should stop playing silly buggers and just return the damn error.
So basically
ret = d_splice_alias(inode, dentry);
if (IS_ERR(ret))
goto bail_unlock;
if (inode) {
if (ret)
dentry = ret;
status = ocfs2_dentry_attach_lock(dentry, inode,
OCFS2_I(dir)->ip_blkno);
if (unlikely(status)) {
if (ret)
dput(ret);
ret = ERR_PTR(status);
}
} else {
ocfs2_dentry_attach_gen(dentry);
}
bail_unlock:
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] ocfs2: update d_splice_alias() return code checking
2025-06-26 3:34 ` Al Viro
@ 2025-06-27 14:19 ` Tetsuo Handa
2025-06-30 1:58 ` Joseph Qi
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2025-06-27 14:19 UTC (permalink / raw)
To: Al Viro, Mark Fasheh, Joel Becker, Joseph Qi, Richard Weinberger,
ocfs2-devel, linux-fsdevel, LKML, Andrew Morton
When commit d3556babd7fa ("ocfs2: fix d_splice_alias() return code
checking") was merged into v3.18-rc3, d_splice_alias() was returning
one of a valid dentry, NULL or an ERR_PTR.
When commit b5ae6b15bd73 ("merge d_materialise_unique() into
d_splice_alias()") was merged into v3.19-rc1, d_splice_alias() started
returning -ELOOP as one of ERR_PTR values.
Now, when syzkaller mounts a crafted ocfs2 filesystem image that hits
d_splice_alias() == -ELOOP case from ocfs2_lookup(), ocfs2_lookup() fails
to handle -ELOOP case and generic_shutdown_super() hits "VFS: Busy inodes
after unmount" message.
Instead of calling ocfs2_dentry_attach_lock() or ocfs2_dentry_attach_gen()
when d_splice_alias() returned an ERR_PTR value, change ocfs2_lookup() to
bail out immediately.
Also, ocfs2_lookup() needs to call dupt() when ocfs2_dentry_attach_lock()
returned an ERR_PTR value.
Reported-by: syzbot <syzbot+1134d3a5b062e9665a7a@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=1134d3a5b062e9665a7a
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/ocfs2/namei.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 99278c8f0e24..f75fd19974bc 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -142,6 +142,8 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
bail_add:
ret = d_splice_alias(inode, dentry);
+ if (IS_ERR(ret))
+ goto bail_unlock;
if (inode) {
/*
@@ -154,13 +156,12 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
* NOTE: This dentry already has ->d_op set from
* ocfs2_get_parent() and ocfs2_get_dentry()
*/
- if (!IS_ERR_OR_NULL(ret))
- dentry = ret;
-
- status = ocfs2_dentry_attach_lock(dentry, inode,
+ status = ocfs2_dentry_attach_lock(ret ? ret : dentry, inode,
OCFS2_I(dir)->ip_blkno);
if (status) {
mlog_errno(status);
+ if (ret)
+ dput(ret);
ret = ERR_PTR(status);
goto bail_unlock;
}
--
2.50.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ocfs2: update d_splice_alias() return code checking
2025-06-27 14:19 ` [PATCH v2] " Tetsuo Handa
@ 2025-06-30 1:58 ` Joseph Qi
2025-06-30 10:21 ` [PATCH v3] " Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Joseph Qi @ 2025-06-30 1:58 UTC (permalink / raw)
To: Tetsuo Handa, Al Viro, Mark Fasheh, Joel Becker,
Richard Weinberger, ocfs2-devel, linux-fsdevel, LKML,
Andrew Morton
On 2025/6/27 22:19, Tetsuo Handa wrote:
> When commit d3556babd7fa ("ocfs2: fix d_splice_alias() return code
> checking") was merged into v3.18-rc3, d_splice_alias() was returning
> one of a valid dentry, NULL or an ERR_PTR.
>
> When commit b5ae6b15bd73 ("merge d_materialise_unique() into
> d_splice_alias()") was merged into v3.19-rc1, d_splice_alias() started
> returning -ELOOP as one of ERR_PTR values.
>
> Now, when syzkaller mounts a crafted ocfs2 filesystem image that hits
> d_splice_alias() == -ELOOP case from ocfs2_lookup(), ocfs2_lookup() fails
> to handle -ELOOP case and generic_shutdown_super() hits "VFS: Busy inodes
> after unmount" message.
>
> Instead of calling ocfs2_dentry_attach_lock() or ocfs2_dentry_attach_gen()
> when d_splice_alias() returned an ERR_PTR value, change ocfs2_lookup() to
> bail out immediately.
>
> Also, ocfs2_lookup() needs to call dupt() when ocfs2_dentry_attach_lock()
> returned an ERR_PTR value.
>
> Reported-by: syzbot <syzbot+1134d3a5b062e9665a7a@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=1134d3a5b062e9665a7a
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> fs/ocfs2/namei.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 99278c8f0e24..f75fd19974bc 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -142,6 +142,8 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
>
> bail_add:
> ret = d_splice_alias(inode, dentry);
> + if (IS_ERR(ret))
> + goto bail_unlock;
>
> if (inode) {
> /*
> @@ -154,13 +156,12 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
> * NOTE: This dentry already has ->d_op set from
> * ocfs2_get_parent() and ocfs2_get_dentry()
> */
> - if (!IS_ERR_OR_NULL(ret))
> - dentry = ret;
> -
I'd like change this to:
if (ret)
dentry = ret;
Instead of using "ret ? ret : dentry" below calling ocfs2_dentry_attach_lock().
> - status = ocfs2_dentry_attach_lock(dentry, inode,
> + status = ocfs2_dentry_attach_lock(ret ? ret : dentry, inode,
> OCFS2_I(dir)->ip_blkno);
> if (status) {
> mlog_errno(status);
> + if (ret)
> + dput(ret);
> ret = ERR_PTR(status);
> goto bail_unlock;
The "goto" here can be eliminated since it has no real effect.
Thanks,
Joseph
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] ocfs2: update d_splice_alias() return code checking
2025-06-30 1:58 ` Joseph Qi
@ 2025-06-30 10:21 ` Tetsuo Handa
2025-07-02 6:08 ` Joseph Qi
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2025-06-30 10:21 UTC (permalink / raw)
To: Joseph Qi, Andrew Morton
Cc: Al Viro, Mark Fasheh, Joel Becker, Richard Weinberger,
ocfs2-devel, linux-fsdevel, LKML
When commit d3556babd7fa ("ocfs2: fix d_splice_alias() return code
checking") was merged into v3.18-rc3, d_splice_alias() was returning
one of a valid dentry, NULL or an ERR_PTR.
When commit b5ae6b15bd73 ("merge d_materialise_unique() into
d_splice_alias()") was merged into v3.19-rc1, d_splice_alias() started
returning -ELOOP as one of ERR_PTR values.
Now, when syzkaller mounts a crafted ocfs2 filesystem image that hits
d_splice_alias() == -ELOOP case from ocfs2_lookup(), ocfs2_lookup() fails
to handle -ELOOP case and generic_shutdown_super() hits "VFS: Busy inodes
after unmount" message.
Instead of calling ocfs2_dentry_attach_lock() or ocfs2_dentry_attach_gen()
when d_splice_alias() returned an ERR_PTR value, change ocfs2_lookup() to
bail out immediately.
Also, ocfs2_lookup() needs to call dupt() when ocfs2_dentry_attach_lock()
returned an ERR_PTR value.
Reported-by: syzbot <syzbot+1134d3a5b062e9665a7a@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=1134d3a5b062e9665a7a
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/ocfs2/namei.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 99278c8f0e24..721580dfce3a 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -142,6 +142,8 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
bail_add:
ret = d_splice_alias(inode, dentry);
+ if (IS_ERR(ret))
+ goto bail_unlock;
if (inode) {
/*
@@ -154,15 +156,16 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
* NOTE: This dentry already has ->d_op set from
* ocfs2_get_parent() and ocfs2_get_dentry()
*/
- if (!IS_ERR_OR_NULL(ret))
+ if (ret)
dentry = ret;
status = ocfs2_dentry_attach_lock(dentry, inode,
OCFS2_I(dir)->ip_blkno);
if (status) {
mlog_errno(status);
+ if (ret)
+ dput(ret);
ret = ERR_PTR(status);
- goto bail_unlock;
}
} else
ocfs2_dentry_attach_gen(dentry);
--
2.47.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ocfs2: update d_splice_alias() return code checking
2025-06-30 10:21 ` [PATCH v3] " Tetsuo Handa
@ 2025-07-02 6:08 ` Joseph Qi
0 siblings, 0 replies; 6+ messages in thread
From: Joseph Qi @ 2025-07-02 6:08 UTC (permalink / raw)
To: Tetsuo Handa, Andrew Morton
Cc: Al Viro, Mark Fasheh, Joel Becker, Richard Weinberger,
ocfs2-devel, linux-fsdevel, LKML
On 2025/6/30 18:21, Tetsuo Handa wrote:
> When commit d3556babd7fa ("ocfs2: fix d_splice_alias() return code
> checking") was merged into v3.18-rc3, d_splice_alias() was returning
> one of a valid dentry, NULL or an ERR_PTR.
>
> When commit b5ae6b15bd73 ("merge d_materialise_unique() into
> d_splice_alias()") was merged into v3.19-rc1, d_splice_alias() started
> returning -ELOOP as one of ERR_PTR values.
>
> Now, when syzkaller mounts a crafted ocfs2 filesystem image that hits
> d_splice_alias() == -ELOOP case from ocfs2_lookup(), ocfs2_lookup() fails
> to handle -ELOOP case and generic_shutdown_super() hits "VFS: Busy inodes
> after unmount" message.
>
> Instead of calling ocfs2_dentry_attach_lock() or ocfs2_dentry_attach_gen()
> when d_splice_alias() returned an ERR_PTR value, change ocfs2_lookup() to
> bail out immediately.
>
> Also, ocfs2_lookup() needs to call dupt() when ocfs2_dentry_attach_lock()
> returned an ERR_PTR value.
>
> Reported-by: syzbot <syzbot+1134d3a5b062e9665a7a@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=1134d3a5b062e9665a7a
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Looks fine to me.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
> fs/ocfs2/namei.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 99278c8f0e24..721580dfce3a 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -142,6 +142,8 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
>
> bail_add:
> ret = d_splice_alias(inode, dentry);
> + if (IS_ERR(ret))
> + goto bail_unlock;
>
> if (inode) {
> /*
> @@ -154,15 +156,16 @@ static struct dentry *ocfs2_lookup(struct inode *dir, struct dentry *dentry,
> * NOTE: This dentry already has ->d_op set from
> * ocfs2_get_parent() and ocfs2_get_dentry()
> */
> - if (!IS_ERR_OR_NULL(ret))
> + if (ret)
> dentry = ret;
>
> status = ocfs2_dentry_attach_lock(dentry, inode,
> OCFS2_I(dir)->ip_blkno);
> if (status) {
> mlog_errno(status);
> + if (ret)
> + dput(ret);
> ret = ERR_PTR(status);
> - goto bail_unlock;
> }
> } else
> ocfs2_dentry_attach_gen(dentry);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-02 6:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-26 2:14 [PATCH] ocfs2: update d_splice_alias() return code checking Tetsuo Handa
2025-06-26 3:34 ` Al Viro
2025-06-27 14:19 ` [PATCH v2] " Tetsuo Handa
2025-06-30 1:58 ` Joseph Qi
2025-06-30 10:21 ` [PATCH v3] " Tetsuo Handa
2025-07-02 6:08 ` Joseph Qi
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).