Re: dm-crypt: Reject sector_size feature if device length is not aligned to it

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Oct 03 2017 at  4:33pm -0400,
Milan Broz <gmazyland@gmail.com> wrote:

> On 10/03/2017 10:08 PM, Mikulas Patocka wrote:
> > 
> > It would be interesting to know, why Milan wants the table load to fail.
> 
> I mentioned this on IRC:
> the only situation I care about in load is that size (dm-table length) is unaligned to optional sector_size.
> create fails in this case, load should imho fail as well. 
> ...
> if we say that dmsetup table output is always directly usable (as a mapping table),
> then why should there be an exception for dmsetup table --inactive? (now it can print apparently invalid mapping)

The .ctr should validate the inactive table and that'll cause load to
fail.

Or dm-crypt could publish block_limits that reflect this optional
sector_size and we'll get create (resume) failure.. which I assume is
what you want to avoid.

> Anyway, I am ok if it fails in resume - but do not keep the device suspended after the fail!

Sounds like we need a patch to resume after failed inactive table load.
Might cause lvm2 to try to resume when there is no need.  But the user
would've already had to suspend and then resume to try to load the
inactive table.  If we resume with the original (working) table it may
surprise the user... will certainly cause lvm2 to fail its table
comparison tests if the resume to old working table is done without
erroring out.

So we'd need to still return error but resume with old table if it
exists... and who is asking for this again?  Just us devs who think
leaving the device suspended is bad form?

The user caused the problem by requesting a malformed table get
used... I'm not sure how I feel about covering for such imprecise users.

> > It could be possible to check the validity of the alignment in the 
> > cryptsetup tool and not attempt to load invalid tables at all. Is there 
> > any reason, why we need to detect the misalignment in the kernel?
> 
> Cryptsetup already rejects such a mapping before even calling dm-ioctl.
> 
> But anyone can use dmsetup tool to do that. I just think that incompatible
> sector vs. device size should be rejected in target constructor.
> (IOW my former patch for dm-crypt that rejects only this exact situation without
> doing more device-related tests like your generalized patch in table_load.)

I'll revisit your patch since it reflects what I first said above (about
the .ctr erroring out as needed).

Not sure why Mikulas is saying all the other targets need this too
(e.g. verity, integrity, etc).

Mike