| Message ID | 20240819025112.2505463-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ocfs2: remove unreasonable unlock | expand |
On 8/19/24 10:51, Lizhi Xu wrote: > There was a lock release before exiting, so remove the unreasonable unlock. > > Reported-and-tested-by: syzbot+ab134185af9ef88dfed5@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=ab134185af9ef88dfed5 > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > --- > fs/ocfs2/buffer_head_io.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c > index cdb9b9bdea1f..e62c7e1de4eb 100644 > --- a/fs/ocfs2/buffer_head_io.c > +++ b/fs/ocfs2/buffer_head_io.c > @@ -235,7 +235,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, > if (bhs[i] == NULL) { > bhs[i] = sb_getblk(sb, block++); > if (bhs[i] == NULL) { > - ocfs2_metadata_cache_io_unlock(ci); > status = -ENOMEM; > mlog_errno(status); > /* Don't forget to put previous bh! */ The fix looks good to me. However, I found another issue in this function. In the for-loop after the 'read_failure' label, the condition '(bh == NULL) && flags includes OCFS2_BH_READAHEAD' is missing. When this contidion is true, this for-loop will call ocfs2_set_buffer_uptodate(ci, bh), which then triggers a NULL pointer access error. If you agree with my analysis, please add the new fix and send a v2 patch. Thanks, Heming
On Tue, 20 Aug 2024 12:04:39 +0800, Heming wrote: > > There was a lock release before exiting, so remove the unreasonable unlock. > > > > Reported-and-tested-by: syzbot+ab134185af9ef88dfed5@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=ab134185af9ef88dfed5 > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > > --- > > fs/ocfs2/buffer_head_io.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c > > index cdb9b9bdea1f..e62c7e1de4eb 100644 > > --- a/fs/ocfs2/buffer_head_io.c > > +++ b/fs/ocfs2/buffer_head_io.c > > @@ -235,7 +235,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, > > if (bhs[i] == NULL) { > > bhs[i] = sb_getblk(sb, block++); > > if (bhs[i] == NULL) { > > - ocfs2_metadata_cache_io_unlock(ci); > > status = -ENOMEM; > > mlog_errno(status); > > /* Don't forget to put previous bh! */ > > The fix looks good to me. However, I found another issue in this function. > > In the for-loop after the 'read_failure' label, the condition > '(bh == NULL) && flags includes OCFS2_BH_READAHEAD' is missing. > When this contidion is true, this for-loop will call ocfs2_set_buffer_uptodate(ci, bh), > which then triggers a NULL pointer access error. > > If you agree with my analysis, please add the new fix and send a v2 patch. I agree with your analysis conclusion, but this is not the same issue as the current one, so I will split it into two patches. The first patch fixes the unbalanced lock issue, and the second patch will be used to fix the UAF problem of BH. BR, Lizhi
diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c index cdb9b9bdea1f..e62c7e1de4eb 100644 --- a/fs/ocfs2/buffer_head_io.c +++ b/fs/ocfs2/buffer_head_io.c @@ -235,7 +235,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, if (bhs[i] == NULL) { bhs[i] = sb_getblk(sb, block++); if (bhs[i] == NULL) { - ocfs2_metadata_cache_io_unlock(ci); status = -ENOMEM; mlog_errno(status); /* Don't forget to put previous bh! */
There was a lock release before exiting, so remove the unreasonable unlock. Reported-and-tested-by: syzbot+ab134185af9ef88dfed5@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=ab134185af9ef88dfed5 Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> --- fs/ocfs2/buffer_head_io.c | 1 - 1 file changed, 1 deletion(-)