linux-kernel - Re: Significant brokenness in DIO loopback path

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: <Yiltk3RWTPJvPJph@T590>
Date:   Thu, 10 Mar 2022 11:16:35 +0800
From:   Ming Lei <ming.lei@...hat.com>
To:     Kent Overstreet <kent.overstreet@...il.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Pavel Begunkov <asml.silence@...il.com>,
        Christoph Hellwig <hch@....de>, Jens Axboe <axboe@...nel.dk>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: Significant brokenness in DIO loopback path

On Wed, Mar 09, 2022 at 08:44:00PM -0500, Kent Overstreet wrote:
> So I'm testing bcachefs with the loopback driver in dio mode, and noticing
> _significant_ brokenness in the bio_iov_iter_get_pages() path and elsewhere.
> 
> 1) We don't check that we're not asking for more pages than we're in the
> original bio
> 
> Noticed this because of another bug:
> 
> 2) the loopback driver appears to never look at the underlying filesystem's
> block size, meaning if the filesystem advertises a block size of 4k the loopback
> device's blocksize will still be 512, and we'll end up issuing IOs the DIO path
> shouldn't allow due to alignment.

I tried to fallback to buffered IO for unaligned dio, it was rejected.

https://lore.kernel.org/linux-block/20211025094437.2837701-1-ming.lei@redhat.com/

Also the ahead of time check may not work as expected because of ioctl
order, I guess that is why you see loop 512 bs even the underlying advertises
big 4k size.

Also loop 512 bs is often useful since the upper FS image may need that.

> 
> 3) iov_iter_bvec_advance() looks like utter nonsense. We're synthesizing a fake
> bvec_iter and never using or even looking at one from the original bio, looking
> at the construction in iov_iter_bvec().
> 
> This is broken; you're assuming you're never going to see bios with partially
> completed bvec_iters, or things are going to explode.
> 
> Try putting a md raid0 on top of two loopback devices with a sub page block
> size, things are just going to explode.
> 
> iov_iter_bvec() needs to be changed to take a bio, not a bvec array, and
> iov_iter_bvec_advance() should probably just call bio_advance() - and
> bio_iov_bvec_set() needs to be changed to just copy bi_iter from the original
> bio into the dest bio. You guys made this way more complicated than it needed to
> be.

Can you share the function in loop.c you are talking? Is it lo_rw_aio()? What is
the exact issue in current way?

If the request has > 1 bio, one bvec array is made for call_write_iter.


Thanks
Ming