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
| ||
|
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
Powered by blists - more mailing lists