linux-ext4 - Re: [RFC PATCH v3 14/61] e2fsck: merge bitmaps after thread completes

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]

Message-ID: <20201124020020.GL132317@mit.edu>
Date:   Mon, 23 Nov 2020 21:00:20 -0500
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Saranya Muruganandam <saranyamohan@...gle.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca,
        Wang Shilong <wshilong@....com>, Li Xi <lixi@....com>
Subject: Re: [RFC PATCH v3 14/61] e2fsck: merge bitmaps after thread completes

On Wed, Nov 18, 2020 at 07:39:00AM -0800, Saranya Muruganandam wrote:
> From: Wang Shilong <wshilong@....com>
> 
> A new method merge_bmap has been added to bitmap operations. But
> only red-black bitmap has that operation now.
> 
> Signed-off-by: Li Xi <lixi@....com>
> Signed-off-by: Wang Shilong <wshilong@....com>
> Signed-off-by: Saranya Muruganandam <saranyamohan@...gle.com>

There are hacks to deal with the fact that only the read-block bitmap
backend supports merge_bmap in this patch series.  Why not implement
that for *all* of the back ends, along with some unit tests; it's
going to be less work than working around failures due to partial
implementation of merge_bmap --- and the workarounds weren't complete
in any case; you can force the use of a particular back-end via
/etc/e2fsck.conf --- see the function
e2fsck_allocate_{block,inode}_bmap() in e2fsck/util.c.

This was mainly way of forcing additional testing of the backend
implementations, and also so we could test/tune whether certain
backends were preferable for certain file systems for different ways
in which block/inode bitmaps are used by e2fsck --- but it's possible
for an sysadmin / SRE to explicitly set them on a production e2fsck,
and it would be preferable if things didn't blow up because we took a
shortcut and didn't implement merge_bmap for all of the bitmap
backends.   (It's *really* not that hard, anyway....)

> +static void e2fsck_pass1_free_bitmap(ext2fs_generic_bitmap *bitmap)
> +{
> +	if (*bitmap) {
> +		ext2fs_free_generic_bmap(*bitmap);
> +		*bitmap = NULL;
> +	}
> +
> +}

I'm not sure why this is needed?  ext2fs_free_Generic_bmap() will
already return if NULL is passed into it.

Also, by the end of the patch series, there are no callers of this
function....

And again, please separate out the libext2fs changes from the e2fsck
changes, do the libext2fs changes first, and there should really be
some unit tests of merge_bmap so we can validate that it works
correctly before we drop it into use with e2fsck.

There should also be better documentation of what dup and dup_allowed
in merge_bmap().

	  	    	    	     - Ted