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 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:   Sat, 19 Nov 2022 09:16:41 +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 07:20AM, Andreas Dilger wrote:
> On Nov 18, 2022, at 05:37, Ritesh Harjani (IBM) <ritesh.list@...il.com> wrote:
> > 
> > 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. 
> 
> You are correct, and it means this code is just not very clear to the reader. I think it
> would make more sense to move the "short_read:" label to the end of the code:
> 
>                   actual = read(...);
>                   if (actual != size)
>                           goto error_short_read;
>                   goto success_unlock;
>         :
>                  actual = read(...);
>                  if (actual != align_size) {
>                            actual = really_read;
>                            buf -= really_read;
>                            size += really_read;
>                            goto error_short_read;
>                  }
>         :
> success_unlock:
>         mutex_unlock(...);
>         return 0;
> 
> error_short_read:
>         if (actual < 0) {
>                  retval = errno;
>                  actual = 0;
>         } else {
>                  retval = EXT2_ET_SHORT_READ;
>         }
> error_unlock:
>         mutex_unlock(...);
> 
> That way the code follows the normal error handling convention and is less likely to be
> surprising to the reader. 

Yes, you are right. I will do the change in the next rev.

-ritesh