| Message ID | 629167c1-9be0-6128-8605-eb02391e821d@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mikulas Patocka |
| Headers | show |
| Series | dm-verity: do forward error correction on metadata I/O errors | expand |
On 2/10/25 4:04 PM, Mikulas Patocka wrote: > Do forward error correction if metadata I/O fails. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Not directly related to this patch, but... please could you also increase the dm-verity version? I just implemented support for the errors-as-corruption dm-verity flags in veritysetup. The version of dm-verity stayed at 1.10 for a very long, and the IO error processing change was a functional change that we would like to detect. We cannot set version retrospectively, but at least now with other changes. (Veritysetup tries the requested flags; if activation fails, it displays a better error message based on the detected target version. That way it works even with backports.) Thanks, Milan > > --- > drivers/md/dm-verity-target.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > Index: linux-2.6/drivers/md/dm-verity-target.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2025-02-04 13:52:45.000000000 +0100 > +++ linux-2.6/drivers/md/dm-verity-target.c 2025-02-10 15:55:42.000000000 +0100 > @@ -324,8 +324,22 @@ static int verity_verify_level(struct dm > &buf, bio->bi_ioprio); > } > > - if (IS_ERR(data)) > - return PTR_ERR(data); > + if (IS_ERR(data)) { > + r = PTR_ERR(data); > + data = dm_bufio_new(v->bufio, hash_block, &buf); > + if (IS_ERR(data)) > + return r; > + if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, > + hash_block, data) == 0) { > + aux = dm_bufio_get_aux_data(buf); > + aux->hash_verified = 1; > + goto release_ok; > + } else { > + dm_bufio_release(buf); > + dm_bufio_forget(v->bufio, hash_block); > + return r; > + } > + } > > aux = dm_bufio_get_aux_data(buf); > > @@ -366,6 +380,7 @@ static int verity_verify_level(struct dm > } > } > > +release_ok: > data += offset; > memcpy(want_digest, data, v->digest_size); > r = 0; >
On Mon, 10 Feb 2025, Milan Broz wrote: > On 2/10/25 4:04 PM, Mikulas Patocka wrote: > > Do forward error correction if metadata I/O fails. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Not directly related to this patch, but... > please could you also increase the dm-verity version? > > I just implemented support for the errors-as-corruption > dm-verity flags in veritysetup. > > The version of dm-verity stayed at 1.10 for a very long, and > the IO error processing change was a functional change that we > would like to detect. > We cannot set version retrospectively, but at least now with other > changes. > > (Veritysetup tries the requested flags; if activation fails, > it displays a better error message based on the detected > target version. That way it works even with backports.) > > Thanks, > Milan OK. From: Mikulas Patocka <mpatocka@redhat.com> Do forward error correction if metadata I/O fails. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- drivers/md/dm-verity-target.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) Index: linux-2.6/drivers/md/dm-verity-target.c =================================================================== --- linux-2.6.orig/drivers/md/dm-verity-target.c 2025-02-10 16:24:56.000000000 +0100 +++ linux-2.6/drivers/md/dm-verity-target.c 2025-02-10 17:43:50.000000000 +0100 @@ -324,8 +324,22 @@ static int verity_verify_level(struct dm &buf, bio->bi_ioprio); } - if (IS_ERR(data)) - return PTR_ERR(data); + if (IS_ERR(data)) { + r = PTR_ERR(data); + data = dm_bufio_new(v->bufio, hash_block, &buf); + if (IS_ERR(data)) + return r; + if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, + hash_block, data) == 0) { + aux = dm_bufio_get_aux_data(buf); + aux->hash_verified = 1; + goto release_ok; + } else { + dm_bufio_release(buf); + dm_bufio_forget(v->bufio, hash_block); + return r; + } + } aux = dm_bufio_get_aux_data(buf); @@ -366,6 +380,7 @@ static int verity_verify_level(struct dm } } +release_ok: data += offset; memcpy(want_digest, data, v->digest_size); r = 0; @@ -1761,7 +1776,7 @@ static struct target_type verity_target .name = "verity", /* Note: the LSMs depend on the singleton and immutable features */ .features = DM_TARGET_SINGLETON | DM_TARGET_IMMUTABLE, - .version = {1, 10, 0}, + .version = {1, 11, 0}, .module = THIS_MODULE, .ctr = verity_ctr, .dtr = verity_dtr,
Hi Mikulas, On Mon, Feb 10, 2025 at 7:04 AM Mikulas Patocka <mpatocka@redhat.com> wrote: > > Do forward error correction if metadata I/O fails. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > --- > drivers/md/dm-verity-target.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > Index: linux-2.6/drivers/md/dm-verity-target.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2025-02-04 13:52:45.000000000 +0100 > +++ linux-2.6/drivers/md/dm-verity-target.c 2025-02-10 15:55:42.000000000 +0100 > @@ -324,8 +324,22 @@ static int verity_verify_level(struct dm > &buf, bio->bi_ioprio); > } > > - if (IS_ERR(data)) > - return PTR_ERR(data); > + if (IS_ERR(data)) { > + r = PTR_ERR(data); > + data = dm_bufio_new(v->bufio, hash_block, &buf); > + if (IS_ERR(data)) > + return r; > + if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, > + hash_block, data) == 0) { > + aux = dm_bufio_get_aux_data(buf); > + aux->hash_verified = 1; > + goto release_ok; > + } else { > + dm_bufio_release(buf); > + dm_bufio_forget(v->bufio, hash_block); > + return r; > + } > + } Don't we still have to check for io->in_bh before trying to correct the error? Overall, it would be nice not to duplicate code here. Should the metadata error correction / handling be moved to a separate function similar to verity_handle_data_hash_mismatch? Sami
On Mon, Feb 10, 2025 at 08:57:55AM -0800, Sami Tolvanen wrote: > Hi Mikulas, > > On Mon, Feb 10, 2025 at 7:04 AM Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > Do forward error correction if metadata I/O fails. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > > --- > > drivers/md/dm-verity-target.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > Index: linux-2.6/drivers/md/dm-verity-target.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2025-02-04 13:52:45.000000000 +0100 > > +++ linux-2.6/drivers/md/dm-verity-target.c 2025-02-10 15:55:42.000000000 +0100 > > @@ -324,8 +324,22 @@ static int verity_verify_level(struct dm > > &buf, bio->bi_ioprio); > > } > > > > - if (IS_ERR(data)) > > - return PTR_ERR(data); > > + if (IS_ERR(data)) { > > + r = PTR_ERR(data); > > + data = dm_bufio_new(v->bufio, hash_block, &buf); > > + if (IS_ERR(data)) > > + return r; > > + if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, > > + hash_block, data) == 0) { > > + aux = dm_bufio_get_aux_data(buf); > > + aux->hash_verified = 1; > > + goto release_ok; > > + } else { > > + dm_bufio_release(buf); > > + dm_bufio_forget(v->bufio, hash_block); > > + return r; > > + } > > + } > > Don't we still have to check for io->in_bh before trying to correct the error? > > Overall, it would be nice not to duplicate code here. Should the > metadata error correction / handling be moved to a separate function > similar to verity_handle_data_hash_mismatch? > It's also incorrect to do this when skip_verified=true, since in that case want_digest (which verity_fec_decode() uses) has not been initialized yet. - Eric
On Mon, 10 Feb 2025, Eric Biggers wrote: > On Mon, Feb 10, 2025 at 08:57:55AM -0800, Sami Tolvanen wrote: > > Hi Mikulas, > > > > On Mon, Feb 10, 2025 at 7:04 AM Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > Do forward error correction if metadata I/O fails. > > > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > > > > --- > > > drivers/md/dm-verity-target.c | 19 +++++++++++++++++-- > > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > > > Index: linux-2.6/drivers/md/dm-verity-target.c > > > =================================================================== > > > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2025-02-04 13:52:45.000000000 +0100 > > > +++ linux-2.6/drivers/md/dm-verity-target.c 2025-02-10 15:55:42.000000000 +0100 > > > @@ -324,8 +324,22 @@ static int verity_verify_level(struct dm > > > &buf, bio->bi_ioprio); > > > } > > > > > > - if (IS_ERR(data)) > > > - return PTR_ERR(data); > > > + if (IS_ERR(data)) { > > > + r = PTR_ERR(data); > > > + data = dm_bufio_new(v->bufio, hash_block, &buf); > > > + if (IS_ERR(data)) > > > + return r; > > > + if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, > > > + hash_block, data) == 0) { > > > + aux = dm_bufio_get_aux_data(buf); > > > + aux->hash_verified = 1; > > > + goto release_ok; > > > + } else { > > > + dm_bufio_release(buf); > > > + dm_bufio_forget(v->bufio, hash_block); > > > + return r; > > > + } > > > + } > > > > Don't we still have to check for io->in_bh before trying to correct the error? dm_bufio_get doesn't return an error code. But adding a test for it doesn't hurt. > > Overall, it would be nice not to duplicate code here. Should the > > metadata error correction / handling be moved to a separate function > > similar to verity_handle_data_hash_mismatch? > > > > It's also incorrect to do this when skip_verified=true, since in that case > want_digest (which verity_fec_decode() uses) has not been initialized yet. > > - Eric Yes, I changed it to return "1" if "skip_unverified" is set. Mikulas
Index: linux-2.6/drivers/md/dm-verity-target.c =================================================================== --- linux-2.6.orig/drivers/md/dm-verity-target.c 2025-02-04 13:52:45.000000000 +0100 +++ linux-2.6/drivers/md/dm-verity-target.c 2025-02-10 15:55:42.000000000 +0100 @@ -324,8 +324,22 @@ static int verity_verify_level(struct dm &buf, bio->bi_ioprio); } - if (IS_ERR(data)) - return PTR_ERR(data); + if (IS_ERR(data)) { + r = PTR_ERR(data); + data = dm_bufio_new(v->bufio, hash_block, &buf); + if (IS_ERR(data)) + return r; + if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, + hash_block, data) == 0) { + aux = dm_bufio_get_aux_data(buf); + aux->hash_verified = 1; + goto release_ok; + } else { + dm_bufio_release(buf); + dm_bufio_forget(v->bufio, hash_block); + return r; + } + } aux = dm_bufio_get_aux_data(buf); @@ -366,6 +380,7 @@ static int verity_verify_level(struct dm } } +release_ok: data += offset; memcpy(want_digest, data, v->digest_size); r = 0;
Do forward error correction if metadata I/O fails. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- drivers/md/dm-verity-target.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)