Re: [RFC PATCH 1/2] dm-verity: don't decode FEC into a dm_bufio page that is already in the cache

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Nov 10, 2026 at 10:08:02PM +0100, Mikulas Patocka wrote:
> On Tue, 10 Nov 2026, 周伟江 (Weijiang Zhou) wrote:
> > Commit 51ba14fb368e ("dm-verity: do forward error correction on metadata
> > I/O errors") handles a failed hash block read by allocating the block with
> > dm_bufio_new() and decoding into it with verity_fec_decode().
> > 
> > dm_bufio_new() inserts the buffer into the v->bufio cache immediately, with
> > no B_READING state and no read_error, i.e. indistinguishable from a
> > successfully read block that merely has not been hash-verified yet. Its
> > contents are whatever the allocator returned (all zeroes with init_on_alloc),
> > and no I/O is ever submitted for it.
> 
> I don't see why this should be a problem.
> 
> If the device returns random data, FEC is supposed to correct it.
> 
> If the device returns an I/O error and we create a buffer with random 
> data, this is similar to the case where the device returned random data - 
> and FEC should also correct it. It doesn't matter whether the random data 
> came from the device or from uninitialized kernel memory.
> 
> So, it seems to me that this patch is just papering over the problem.

dm-verity does use FEC to try to recover from I/O errors, but this
doesn't mean that I/O errors and corruption are interchangeable and
handled in the same way.  See the difference at the end of
verity_handle_data_hash_mismatch(), for example.  It calls
verity_handle_err() only on actual corruption, not an I/O error.  That
triggers the corruption policy controlled by 'v->mode', which is
different from the I/O error policy controlled by 'v->error_mode'.

> > FEC then reads every other block of the RS codeword and, for blocks on the
> > data device, calls verity_hash_for_block() on them to decide whether they
> > are erasures. That walks the hash tree through the same v->bufio. If one of
> > those data blocks is covered by the hash block being recovered (or by a
> > descendant of it, when it is an upper level block), the nested
> 
> This is the real problem that should be fixed. Correcting a block 
> shouldn't depend on the content of the block being corrected.

There does seem to be a bug where when FEC is trying to read data block
hashes to correct a block, it can detect corruption in the hash tree and
be unable to correct it due to FEC being non-recursive.  That triggers
the corruption policy, when really it shouldn't and FEC should just
consider erasure information unavailable.  That is fixable.

But it is not the root cause problem here.  The root cause problem is
that the call to dm_bufio_new() injects uninitialized data into the hash
block cache as if it came from a successful read.  That turns an I/O
error into corruption, making the corruption policy take effect merely
due to an I/O error.  This is not limited to FEC reading the data block
erasures, either; any concurrent reader can trip over this corruption.

It seems that dm_bufio_new() was designed for writers that overwrite the
full block without reading any of it while also excluding concurrent
readers, e.g. using locks.  That just does not work here, unfortunately.

Using a kmalloc() buffer (this patch) kind of works but is a hack
because it doesn't actually populate the cache at all.  I think the
correct solution would be to have the FEC output initially just go into
fio->output, then populate the cache atomically with the data.  Seems
that dm-bufio.c will need a new function for that.

- Eric