Re: [PATCH] bcachefs: fix iov_iter count underflow on sub-block dio read

[Su Yue <l@damenly.org>]

On Wed 14 Feb 2024 at 13:13, Brian Foster <bfoster@redhat.com> 
wrote:

> On Fri, Feb 09, 2024 at 10:39:43AM +0800, Su Yue wrote:
>>
>> On Mon 05 Feb 2024 at 14:38, Brian Foster <bfoster@redhat.com> 
>> wrote:
>>
>> > On Mon, Feb 05, 2024 at 02:15:36PM -0500, Kent Overstreet 
>> > wrote:
>> > > On Mon, Feb 05, 2024 at 10:48:14AM -0500, Brian Foster 
>> > > wrote:
>> > > > bch2_direct_IO_read() checks the request offset and size 
>> > > > for >
>> > > sector
>> > > > alignment and then falls through to a couple calculations 
>> > > > to >
>> > > shrink
>> > > > the size of the request based on the inode size. The 
>> > > > problem > is
>> > > that
>> > > > these checks round up to the fs block size, which runs 
>> > > > the > risk
>> > > of
>> > > > underflowing iter->count if the block size happens to be 
>> > > > > large
>> > > > enough. This is triggered by fstest generic/361 with a 4k 
>> > > > > block
>> > > > size, which subsequently leads to a crash.
>> > > >
>> > > > After some discussion, the original purpose of the 
>> > > > shorten > logic
>> > > in this
>> > > > path is not totally clear. It appears to be intended as 
>> > > > an >
>> > > optimization
>> > > > of limited value, so simplify things and avoid the 
>> > > > underflow >
>> > > problem by
>> > > > removing it.
>> > >
>> > > Thinking about this a bit more...
>> > >
>> > > All that we're avoiding with the shorten is zeroing out the 
>> > > rest of
>> > > the
>> > > read, which will happen in bch2_read_extent(); we won't be 
>> > > issuing
>> > > any
>> > > IO, since that would require real data extents above 
>> > > i_size, which
>> > > is of
>> > > course not allowed.
>> > >
>> > > But zeroing out the rest of the read is actual work that's 
>> > > being
>> > > avoided, since if we return a short read (which is what 
>> > > we're doing
>> > > here) it's undefined what happens to the rest of the 
>> > > buffer.
>> > >
>> > > So... maybe we should keep it, for the crazy but inevitable
>> > > application
>> > > that uses a 1MB read buffer for everything, including 
>> > > O_DIRECT reads
>> > > to
>> > > 4k files?
>> > >
>> >
>> > I had another change around that basically just protected 
>> > against the
>> > underflow. I.e., something along the lines of the following, 
>> > to also
>> > cover the additional use of shorten later in the function:
>> >
>> > 	if (shorten >= iter->count)
>> > 		shorten = 0;
>> >

Sorry for the misleading reply.
I mean the check of shorten is efficient and simpler which looks 
good to me.

>>
>> The change is a clean fix without breaking logic in 
>> bch2_direct_IO_read.
>> If without shroten, the next loop counting on iter->count must 
>> be altered.
>> I'd like the simpler way.
>>
>
> Hi Su,
>
> I'm not quite following your comment on the "next loop counting 
> on
> iter->count." Can you elaborate? Also, which option are you 
> calling more
> simple? :) Thanks.
>
I think I just explained Kent's opinion in another way. The loop 
is

fs/bcachefs/fs-io-direct.c:
static int bch2_direct_IO_read(struct kiocb *req, struct iov_iter 
*iter) {
91         iter->count -= shorten;
...
130         while (iter->count) {
                ...
                bch2_read()
            }

            iter->count += shorten;
}

bch2_direct_IO_read() does all boundary check for dio. If shorten 
is
removed and no other condition in the loop, it brings more bios,
more btree searches in bch2_read[_extent].

--
Su
> Brian
>
>> --
>> Su
>>
>> > ... and IIRC that seemed to work as well. But I'll probably 
>> > have to take
>> > a closer look at that large buffer case to grok what's 
>> > happening and the
>> > original purpose of this logic.
>> >
>> > Brian
>>