Milan,

I and Dmitriy tries to fix a problem with BIO split with integrity data 
attached.
This is case, integrity generated/attached before bio_submit over raid device 
(md stack in my case) and bio is subject of bio_integrity_advance.
But bio_integrity_advance incorrectly split an integrity data, as it's assume 
integrity iterator in the 512b blocks.
( bip->bip_iter.bi_sector += bytes_done >> 9 )
But t10 generate function (t10_pi_generate) increase a iterator by 1 for each 
integrity interval in the t10_pi_generate function.
(        for (i = 0 ; i < iter->data_size ; i += iter->interval) {
                iter->seed++;
        }
)
It's good for the scsi with 512b blocks, but bad for the nvme with 4k block 
size.

So two solutions exist 
1) my solution - lets fix an t10_pi_generate / t10_pi_verify /  
t10_pi_type1_prepare to increase iterator by value related to real integrity 
block in the 512b blocks.
2) Martin solution, just change an bio_integrity_advance function to make 
iterator shit in this integrity data blocks units instead of 512b blocks.

Alex

On 04/02/2022, 10:44, "Milan Broz" <gmazyl...@gmail.com> wrote:

    On 04/02/2022 04:44, Martin K. Petersen wrote:
    > 
    > Dmitry,
    > 
    >> My only concern is dm_crypt target which operates on bip_iter directly
    >> with the code copy-pasted from bio_integrity_advance:
    >>
    >> static int dm_crypt_integrity_io_alloc(struct dm_crypt_io *io, struct 
bio *bio)
    >> {
    >>  struct bio_integrity_payload *bip;
    >>  unsigned int tag_len;
    >>  int ret;
    >>
    >>  if (!bio_sectors(bio) || !io->cc->on_disk_tag_size)
    >>          return 0;
    >>
    >>  bip = bio_integrity_alloc(bio, GFP_NOIO, 1);
    >>  if (IS_ERR(bip))
    >>          return PTR_ERR(bip);
    >>
    >>  tag_len = io->cc->on_disk_tag_size * (bio_sectors(bio) >> 
io->cc->sector_shift);
    >>
    >>  bip->bip_iter.bi_size = tag_len;
    >>  bip->bip_iter.bi_sector = io->cc->start + io->sector;
    >>                 ^^^
    >>
    >>  ret = bio_integrity_add_page(bio, virt_to_page(io->integrity_metadata),
    >>                               tag_len, 
offset_in_page(io->integrity_metadata));
    >> ...
    >> }
    > 
    > I copied Milan and Mike who are more familiar with the dm-drypt internals.

    Hi,

    What's the problem here you are trying to fix?
    Even if I read linux-block posts, I do not understand the context...

    Anyway, cc to Mikulas and dm-devel, as dm-integrity/dm-crypt is
    the major user of bio_integrity here.

    If you touch the code, please be sure you run cryptsetup testsuite
    with the integrity tests.
    (IOW integritysetup tests and LUKS2 with authenticated encryption
    that uses dm-crypt over dm-integrity.)

    All we need is that dm-integrity can process bio integrity data
    directly. (I know some people do not like it, but this was
    the most "elegant" solution here.)

    Here dm-crypt uses the data for authenticated encryption (additional
    auth tag in bio field), so because dm-crypt owns bio, integrity data
    must be allocated in dm-crypt (stacked over dm-integrity).

    Milan


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Reply via email to