linux-kernel - Re: [dm-devel] [PATCH] dm-integrity: Prevent RMW for full tag area writes

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

Message-ID: <20200227130837.2707a09e@luklap>
Date:   Thu, 27 Feb 2020 13:08:37 +0100
From:   Lukas Straub <lukasstraub2@....de>
To:     Mikulas Patocka <mpatocka@...hat.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        dm-devel <dm-devel@...hat.com>,
        Mike Snitzer <snitzer@...hat.com>,
        Alasdair Kergon <agk@...hat.com>
Subject: Re: [dm-devel] [PATCH] dm-integrity: Prevent RMW for full tag area
 writes

On Wed, 26 Feb 2020 09:14:31 -0500 (EST)
Mikulas Patocka <mpatocka@...hat.com> wrote:

> On Wed, 26 Feb 2020, Lukas Straub wrote:
>
> > > > -		data = dm_bufio_read(ic->bufio, *metadata_block, &b);
> > > > -		if (IS_ERR(data))
> > > > -			return PTR_ERR(data);
> > > > +		/* Don't read tag area from disk if we're going to overwrite it completely */
> > > > +		if (op == TAG_WRITE && *metadata_offset == 0 && total_size >= ic->metadata_run) {
> > >
> > > Hi
> > >
> > > This is incorrect logic because ic->metadata_run is in the units of
> > > 512-byte sectors and total_size is in bytes.
> > >
> > > If I correct the bug and change it to "if (op == TAG_WRITE &&
> > > *metadata_offset == 0 && total_size >= ic->metadata_run << SECTOR_SHIFT)",
> > > then the benchmark doesn't show any performance advantage at all.
> >
> > Uh oh, looking at it again i have mixed up sectors/bytes elsewhere too.
> > Actually, could we rewrite this check like
> >  total_size >= (1U << SECTOR_SHIFT << ic->log2_buffer_sectors)?
> > this should work, right?
> > So we only have to overwrite part of the tag area, as long as it's whole sectors.
> >
> > > You would need much bigger bios to take advantage for this - for example,
> > > if we have 4k block size and 64k metadata buffer size and 4-byte crc32,
> > > there are 65536/4=16384 tags in one metadata buffer and we would need
> > > 16384*4096=64MiB bio to completely overwrite the metadata buffer. Such big
> > > bios are not realistic.
> >
> > What prevents us from using a single sector as the tag area? (Which was
>
> Single sector writes perform badly on SSDs (and on disks with 4k physical
> sector size). We would need at least 4k.
People with SSDs can still use a large tag area.

> There's another problem - using smaller metadata blocks will degrade read
> performance, because we would need to issue more requests to read the
> metadata.
We can use prefetching, dm-bufio supports that.

> > my assumption with this patch) Then we would have (with 512b sectors)
> > 512/4 = 128 tags = 64k bio, which is still below the optimal write size
>
> 4096/4*4096 = 4MiB - it may be possible, but it's still large.

We don't have to fill whole sector with metadata, we can for example use just the first
512 bytes (giving 512/4*4096 = 512k writes).
The space overhead is negligible:
For 1T of data we have 1G of metadata if we fill the whole sector.
If we use just the first 512 bytes, we get 8G of metadata.

Regards,
Lukas Straub

> > of raid5/6. I just tried to accomplish this, but there seems to be
> > minimum limit of interleave_sectors.
> >
> > Regards,
> > Lukas Straub
>
> Mikulas
>