On Tue, Oct 03 2017 at 4:33pm -0400, Milan Broz <gmazyland@xxxxxxxxx> 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 -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
