linux-ext4 - Re: [RFCv1 01/72] e2fsck: Fix unbalanced mutex unlock for BOUNCE

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Nov 2022 17:07:11 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org,
        Harshad Shirwadkar <harshadshirwadkar@...il.com>,
        Wang Shilong <wshilong@....com>,
        Andreas Dilger <adilger.kernel@...ger.ca>, Li Xi <lixi@....com>
Subject: Re: [RFCv1 01/72] e2fsck: Fix unbalanced mutex unlock for BOUNCE_MTX

On 22/11/18 04:34AM, Andreas Dilger wrote:
> On Nov 7, 2022, at 06:22, Ritesh Harjani (IBM) <ritesh.list@...il.com> wrote:
> > 
> > f_crashdisk test failed with UNIX_IO_FORCE_BOUNCE=yes due to unbalanced
> > mutex unlock in below path.
> > 
> > This patch fixes it.
> > 
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
> > ---
> > lib/ext2fs/unix_io.c | 1 -
> > 1 file changed, 1 deletion(-)
> > 
> > diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> > index e53db333..5b894826 100644
> > --- a/lib/ext2fs/unix_io.c
> > +++ b/lib/ext2fs/unix_io.c
> > @@ -305,7 +305,6 @@ bounce_read:
> >    while (size > 0) {
> >        actual = read(data->dev, data->bounce, align_size);
> >        if (actual != align_size) {
> > -            mutex_unlock(data, BOUNCE_MTX);
> 
> This patch doesn't show enough context, but AFAIK this is jumping before mutex_down()
> is called, so this *should* be correct as is?

Thanks for the review, Andreas.

Yeah, the patch diff above is not sufficient since it doesn't share enuf
context.
But essentially when "actual" is not equal to "align_size", then in this if
condition it goes to label "short_read:", which always goto error_unlock,
where we anyways call mutex_unlock()

Looking at a lot of labels in this function, this definitely looks like 
something which can be cleaned up ("raw_read_blk()"). 
I will add that to my list of todos. 

I have also shared the threadsan warning which detects the unbalanced mutex 
unlock here [1]

[1]: https://lore.kernel.org/linux-ext4/cover.1667822611.git.ritesh.list@gmail.com/T/#md75b3ccb146e4433653bc2d7dd01329a9757ea26

-ritesh