Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Nov 2022 17:07:11 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     Theodore Ts'o <tytso@mit.edu>, linux-ext4@vger.kernel.org,
        Harshad Shirwadkar <harshadshirwadkar@gmail.com>,
        Wang Shilong <wshilong@ddn.com>,
        Andreas Dilger <adilger.kernel@dilger.ca>, Li Xi <lixi@ddn.com>
Subject: Re: [RFCv1 01/72] e2fsck: Fix unbalanced mutex unlock for BOUNCE_MTX
Message-ID: <20221118113711.qby7gtky5k36f7vd@riteshh-domain>
References: <febbbd17b3cf4201aaae24e4adb61e4f8a80e9c9.1667822611.git.ritesh.list@gmail.com>
 <0F4372E0-A232-4DC8-81CE-54D8C0921D1C@dilger.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0F4372E0-A232-4DC8-81CE-54D8C0921D1C@dilger.ca>
Precedence: bulk

On 22/11/18 04:34AM, Andreas Dilger wrote:
> On Nov 7, 2022, at 06:22, Ritesh Harjani (IBM) <ritesh.list@gmail.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@gmail.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