* [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it @ 2017-09-13 13:45 Milan Broz 2017-09-30 18:31 ` Milan Broz 0 siblings, 1 reply; 12+ messages in thread From: Milan Broz @ 2017-09-13 13:45 UTC (permalink / raw) To: dm-devel; +Cc: mpatocka, Milan Broz If a crypt mapping uses optional sector_size feature, additional restrictions to mapped device segment size must be applied in constructor, otherwise the device activation will fail later. Signed-off-by: Milan Broz <gmazyland@gmail.com> --- drivers/md/dm-crypt.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 54aef8ed97db..488ecd0b1bd0 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -2584,6 +2584,10 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar ti->error = "Invalid feature value for sector_size"; return -EINVAL; } + if (ti->len & ((cc->sector_size >> SECTOR_SHIFT) - 1)) { + ti->error = "Device size is not multiple of sector_size feature"; + return -EINVAL; + } cc->sector_shift = __ffs(cc->sector_size) - SECTOR_SHIFT; } else if (!strcasecmp(opt_string, "iv_large_sectors")) set_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags); -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-09-13 13:45 [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it Milan Broz @ 2017-09-30 18:31 ` Milan Broz 2017-10-02 14:43 ` Mikulas Patocka 0 siblings, 1 reply; 12+ messages in thread From: Milan Broz @ 2017-09-30 18:31 UTC (permalink / raw) To: dm-devel; +Cc: Mike Snitzer, Mikulas Patocka On 09/13/2017 03:45 PM, Milan Broz wrote: > If a crypt mapping uses optional sector_size feature, additional > restrictions to mapped device segment size must be applied in constructor, > otherwise the device activation will fail later. Hi, we had some discussion with Mikulas if this check should be better in generic DM code. I think that for this case it is not a good idea - dm-crypt can increase encryption sector size during load (it is stupid to do, but I see no reason why to block it). And then only constructor of the target itself know what is possible and what should be rejected. Anyway, there is a short reproducer what this patch solves: Create simple mapping with 4096 encryption sector: # dmsetup create test --table "0 8 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096" Now load new unaligned-length table (this should fail!) # dmsetup load test --table "0 9 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096" Inactive table is apparently accepted: # dmsetup table --inactive test: 0 9 crypt cipher_null 0 0 8:16 0 1 sector_size:4096 And now, resume fails, keeping the device in suspended state afterward: # dmsetup resume test device-mapper: resume ioctl on test failed: Invalid argument Command failed kernel: device-mapper: table: 254:0: len=9 not aligned to h/w logical block size 4096 of sdb # dmsetup info -c Name Maj Min Stat Open Targ Event UUID test 254 0 L-sw 0 1 0 With the patch applied, the load step correctly fails: # dmsetup load test --table "0 9 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096" device-mapper: reload ioctl on test failed: Invalid argument kernel: device-mapper: table: 254:0: crypt: Device size is not multiple of sector_size feature Please consider this for 4.14 (and stable 4.12+ perhaps). Thanks, Milan > > Signed-off-by: Milan Broz <gmazyland@gmail.com> > --- > drivers/md/dm-crypt.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index 54aef8ed97db..488ecd0b1bd0 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -2584,6 +2584,10 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar > ti->error = "Invalid feature value for sector_size"; > return -EINVAL; > } > + if (ti->len & ((cc->sector_size >> SECTOR_SHIFT) - 1)) { > + ti->error = "Device size is not multiple of sector_size feature"; > + return -EINVAL; > + } > cc->sector_shift = __ffs(cc->sector_size) - SECTOR_SHIFT; > } else if (!strcasecmp(opt_string, "iv_large_sectors")) > set_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-09-30 18:31 ` Milan Broz @ 2017-10-02 14:43 ` Mikulas Patocka 2017-10-03 6:27 ` Milan Broz 2017-10-03 12:05 ` Alasdair G Kergon 0 siblings, 2 replies; 12+ messages in thread From: Mikulas Patocka @ 2017-10-02 14:43 UTC (permalink / raw) To: Milan Broz; +Cc: dm-devel, Mike Snitzer, Alasdair G. Kergon On Sat, 30 Sep 2017, Milan Broz wrote: > On 09/13/2017 03:45 PM, Milan Broz wrote: > > If a crypt mapping uses optional sector_size feature, additional > > restrictions to mapped device segment size must be applied in constructor, > > otherwise the device activation will fail later. > > Hi, > > we had some discussion with Mikulas if this check should be better in generic DM code. > > I think that for this case it is not a good idea - dm-crypt can increase > encryption sector size during load (it is stupid to do, but I see no reason why to block it). > And then only constructor of the target itself know what is possible and what should be rejected. The same argument also applies to verity, integrity and zoned target. I think it should be tested in the generic dm code, not duplicated in these targets. Here I send a patch that checks invalid limits when the table is loaded and aborts the table load ioctl with an error. > Anyway, there is a short reproducer what this patch solves: > > Create simple mapping with 4096 encryption sector: > > # dmsetup create test --table "0 8 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096" > > Now load new unaligned-length table (this should fail!) > # dmsetup load test --table "0 9 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096" > > Inactive table is apparently accepted: > # dmsetup table --inactive > test: 0 9 crypt cipher_null 0 0 8:16 0 1 sector_size:4096 > > And now, resume fails, keeping the device in suspended state afterward: > > # dmsetup resume test > device-mapper: resume ioctl on test failed: Invalid argument > Command failed > kernel: device-mapper: table: 254:0: len=9 not aligned to h/w logical block size 4096 of sdb BTW. if you resume it twice, it will continue working with the old table. > # dmsetup info -c > Name Maj Min Stat Open Targ Event UUID > test 254 0 L-sw 0 1 0 > > With the patch applied, the load step correctly fails: > # dmsetup load test --table "0 9 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096" > device-mapper: reload ioctl on test failed: Invalid argument > kernel: device-mapper: table: 254:0: crypt: Device size is not multiple of sector_size feature > > Please consider this for 4.14 (and stable 4.12+ perhaps). > > Thanks, > Milan From: Mikulas Patocka <mpatocka@redhat.com> Subject: dm: check invalid limits when table is created Device mapper checks invalid limits when resuming a device and swapping a table, however it may be too late becuase it makes the resume ioctl fail. This patch checks the limits when a table is loaded, so that the table load ioctl will fail. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- drivers/md/dm-ioctl.c | 5 +++++ 1 file changed, 5 insertions(+) Index: linux-2.6/drivers/md/dm-ioctl.c =================================================================== --- linux-2.6.orig/drivers/md/dm-ioctl.c +++ linux-2.6/drivers/md/dm-ioctl.c @@ -1308,6 +1308,7 @@ static int table_load(struct file *filp, struct dm_table *t, *old_map = NULL; struct mapped_device *md; struct target_type *immutable_target_type; + struct queue_limits dummy_limits; md = find_device(param); if (!md) @@ -1349,6 +1350,10 @@ static int table_load(struct file *filp, goto err_unlock_md_type; } + r = dm_calculate_queue_limits(t, &dummy_limits); + if (r) + goto err_unlock_md_type; + dm_unlock_md_type(md); /* stage inactive table */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-02 14:43 ` Mikulas Patocka @ 2017-10-03 6:27 ` Milan Broz 2017-10-03 12:05 ` Alasdair G Kergon 1 sibling, 0 replies; 12+ messages in thread From: Milan Broz @ 2017-10-03 6:27 UTC (permalink / raw) To: Mikulas Patocka; +Cc: dm-devel, Mike Snitzer, Alasdair G. Kergon On 10/02/2017 04:43 PM, Mikulas Patocka wrote: > > > On Sat, 30 Sep 2017, Milan Broz wrote: > >> On 09/13/2017 03:45 PM, Milan Broz wrote: >>> If a crypt mapping uses optional sector_size feature, additional >>> restrictions to mapped device segment size must be applied in constructor, >>> otherwise the device activation will fail later. >> >> Hi, >> >> we had some discussion with Mikulas if this check should be better in generic DM code. >> >> I think that for this case it is not a good idea - dm-crypt can increase >> encryption sector size during load (it is stupid to do, but I see no reason why to block it). >> And then only constructor of the target itself know what is possible and what should be rejected. > > The same argument also applies to verity, integrity and zoned target. I > think it should be tested in the generic dm code, not duplicated in these > targets. > > Here I send a patch that checks invalid limits when the table is loaded > and aborts the table load ioctl with an error. ok, I thought it does not work if we change sector size between active and inactive table, but your approach apparently works even for this case. > From: Mikulas Patocka <mpatocka@redhat.com> > Subject: dm: check invalid limits when table is created > > Device mapper checks invalid limits when resuming a device and swapping a > table, however it may be too late becuase it makes the resume ioctl fail. > This patch checks the limits when a table is loaded, so that the table > load ioctl will fail. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Tested-by: Milan Broz <gmazyland@gmail.com> Thanks! Milan > > --- > drivers/md/dm-ioctl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > Index: linux-2.6/drivers/md/dm-ioctl.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-ioctl.c > +++ linux-2.6/drivers/md/dm-ioctl.c > @@ -1308,6 +1308,7 @@ static int table_load(struct file *filp, > struct dm_table *t, *old_map = NULL; > struct mapped_device *md; > struct target_type *immutable_target_type; > + struct queue_limits dummy_limits; > > md = find_device(param); > if (!md) > @@ -1349,6 +1350,10 @@ static int table_load(struct file *filp, > goto err_unlock_md_type; > } > > + r = dm_calculate_queue_limits(t, &dummy_limits); > + if (r) > + goto err_unlock_md_type; > + > dm_unlock_md_type(md); > > /* stage inactive table */ > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-02 14:43 ` Mikulas Patocka 2017-10-03 6:27 ` Milan Broz @ 2017-10-03 12:05 ` Alasdair G Kergon 2017-10-03 18:08 ` Mike Snitzer 1 sibling, 1 reply; 12+ messages in thread From: Alasdair G Kergon @ 2017-10-03 12:05 UTC (permalink / raw) To: Mikulas Patocka; +Cc: dm-devel, Mike Snitzer, Milan Broz On Mon, Oct 02, 2017 at 10:43:56AM -0400, Mikulas Patocka wrote: > Here I send a patch that checks invalid limits when the table is loaded > and aborts the table load ioctl with an error. The code does not already do this because it needs to avoid unnecessary failures if any characteristics of the devices underneath change after the 'load' but before the 'resume' (as can happen, for example, when a tree of devices gets updated by performing all the loads before all the resumes). Alasdair ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-03 12:05 ` Alasdair G Kergon @ 2017-10-03 18:08 ` Mike Snitzer 2017-10-03 19:09 ` Alasdair G Kergon 0 siblings, 1 reply; 12+ messages in thread From: Mike Snitzer @ 2017-10-03 18:08 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: dm-devel, Mikulas Patocka, Milan Broz On Tue, Oct 03 2017 at 8:05am -0400, Alasdair G Kergon <agk@redhat.com> wrote: > On Mon, Oct 02, 2017 at 10:43:56AM -0400, Mikulas Patocka wrote: > > Here I send a patch that checks invalid limits when the table is loaded > > and aborts the table load ioctl with an error. > > The code does not already do this because it needs to avoid unnecessary > failures if any characteristics of the devices underneath change after > the 'load' but before the 'resume' (as can happen, for example, when a > tree of devices gets updated by performing all the loads before all the > resumes). Not sure why you're putting such value on that behaviour. The earlier we catch invalid tables the better off we are. Failing at resume time sucks (always has). Could be there is some specific scenario where lvm2 is leveraging this historic DM "feature" but it'd be nice to revalidate the need. Mike ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-03 18:08 ` Mike Snitzer @ 2017-10-03 19:09 ` Alasdair G Kergon 2017-10-03 20:08 ` Mikulas Patocka 0 siblings, 1 reply; 12+ messages in thread From: Alasdair G Kergon @ 2017-10-03 19:09 UTC (permalink / raw) To: Mike Snitzer; +Cc: dm-devel, Mikulas Patocka, Milan Broz, Alasdair G Kergon On Tue, Oct 03, 2017 at 02:08:04PM -0400, Mike Snitzer wrote: > Not sure why you're putting such value on that behaviour. The earlier > we catch invalid tables the better off we are. Failing at resume time > sucks (always has). Validation code shouldn't be making assumptions about things that lie completely outside its control and falsely failing operations that would actually succeed if they were allowed to proceed. The existing kernel/userspace interface does not require userspace to load devices in any particular sequence. We could have provided a tree-based kernel/userspace interface with stronger requirements like these, but the fact is, we haven't. Perhaps we will one day. As a minimum, you would need to change the patch to validate against the inactive tables of underlying devices instead of the live ones - i.e. assume that userspace already loaded all the underlying devices (and will resume them all before the one being validated gets resumed). Currently no such ordering requirement is imposed on userspace, so you'd also need a compatibility flag to enable the stronger contraints. This patch can break valid userspace code. Alasdair ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-03 19:09 ` Alasdair G Kergon @ 2017-10-03 20:08 ` Mikulas Patocka 2017-10-03 20:33 ` Milan Broz 0 siblings, 1 reply; 12+ messages in thread From: Mikulas Patocka @ 2017-10-03 20:08 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: dm-devel, Milan Broz, Mike Snitzer On Tue, 3 Oct 2017, Alasdair G Kergon wrote: > On Tue, Oct 03, 2017 at 02:08:04PM -0400, Mike Snitzer wrote: > > Not sure why you're putting such value on that behaviour. The earlier > > we catch invalid tables the better off we are. Failing at resume time > > sucks (always has). > > Validation code shouldn't be making assumptions about things that lie > completely outside its control and falsely failing operations that would > actually succeed if they were allowed to proceed. The existing > kernel/userspace interface does not require userspace to load devices in > any particular sequence. We could have provided a tree-based kernel/userspace > interface with stronger requirements like these, but the fact is, we haven't. > Perhaps we will one day. > > As a minimum, you would need to change the patch to validate against the > inactive tables of underlying devices instead of the live ones - i.e. assume > that userspace already loaded all the underlying devices (and will resume them > all before the one being validated gets resumed). Currently no such > ordering requirement is imposed on userspace, so you'd also need a > compatibility flag to enable the stronger contraints. > > This patch can break valid userspace code. > > Alasdair It would be interesting to know, why Milan wants the table load to fail. 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? Mikulas ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-03 20:08 ` Mikulas Patocka @ 2017-10-03 20:33 ` Milan Broz 2017-10-03 21:18 ` Mike Snitzer 0 siblings, 1 reply; 12+ messages in thread From: Milan Broz @ 2017-10-03 20:33 UTC (permalink / raw) To: Mikulas Patocka, Alasdair G Kergon; +Cc: dm-devel, Mike Snitzer 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) Anyway, I am ok if it fails in resume - but do not keep the device suspended after the fail! > 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.) Milan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-03 20:33 ` Milan Broz @ 2017-10-03 21:18 ` Mike Snitzer 2017-10-04 6:45 ` Milan Broz 0 siblings, 1 reply; 12+ messages in thread From: Mike Snitzer @ 2017-10-03 21:18 UTC (permalink / raw) To: Milan Broz; +Cc: dm-devel, Mikulas Patocka, Alasdair G Kergon 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-03 21:18 ` Mike Snitzer @ 2017-10-04 6:45 ` Milan Broz 2017-10-04 15:05 ` Mike Snitzer 0 siblings, 1 reply; 12+ messages in thread From: Milan Broz @ 2017-10-04 6:45 UTC (permalink / raw) To: Mike Snitzer; +Cc: dm-devel, Mikulas Patocka, Alasdair G Kergon On 10/03/2017 11:18 PM, Mike Snitzer wrote: > 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. And that's exactly what is the former patch doing - we introduced a new parameter that has new limitations, we should fix constructor. That's all I want :-) > 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. I think it is doing it already, that's why Mikulas patch works (it validates reported device physical sector sizes, not the internal encryption block parameter). >> 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. IMHO there are some basic principles that should be invariant. We can say, in a specific case, that we intentionally violate it, because the risk is negligible. But we should not be trapped in "normalization of deviation". The rule "only developers (or imprecise users) report this so it is not serious problem yet" sounds like this approach. For me, these are invariants (must be enforced in kernel) that still make sense: 1) Target constructor must validate table and must fail if options are incompatible. 2) The "dmsetup table [--inactive]" (active and inactive table in-kernel) must not contain incompatible options. (I am not saying anything about in-table devices attributes validation, these can change before resume.) It implies that it should be *load* operation that fails and not resume, causing that invalid inactive table is never accepted. Resume still can fail later for other reasons (underlying device disappeared and similar). And this is *different* problem - should it reactivate old table if possible? Dunno. I just can cay that suspended device are nasty and causes many problems so I prefer our tools minimize situations when it happens. For the reported problem - there are no users yet, only developers (me) did that mistake and can report it. I can imagine that the same mistake can be seen in scripts that automatically reacts to device size changes. Up to now, there were no limitations to device size alignment. I have no idea how many scripts people invented. I have fixed cryptsetup only. If an unmodified script fails and keeps underlying device in suspended state, for administrator, it would be really irritating - it should fail and not block the device. Sorry for the rant but I couldn't resist. Back in quiet mode now ;-) Milan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dm-crypt: Reject sector_size feature if device length is not aligned to it 2017-10-04 6:45 ` Milan Broz @ 2017-10-04 15:05 ` Mike Snitzer 0 siblings, 0 replies; 12+ messages in thread From: Mike Snitzer @ 2017-10-04 15:05 UTC (permalink / raw) To: Milan Broz; +Cc: dm-devel, Mikulas Patocka, Alasdair G Kergon On Wed, Oct 04 2017 at 2:45am -0400, Milan Broz <gmazyland@gmail.com> wrote: > On 10/03/2017 11:18 PM, Mike Snitzer wrote: > > 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. > > And that's exactly what is the former patch doing - we introduced a new parameter that > has new limitations, we should fix constructor. That's all I want :-) Yeap, I've staged your fix.. enough with all this debate. See: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-4.14/dm&id=783874b050768d361239e444ba0fa396bb6d463f (but yes I read your "rant".. I agreed with all you said.. but it lacked the qualities of a flaming rant.. you must be happier ;) ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-10-04 15:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-13 13:45 [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it Milan Broz 2017-09-30 18:31 ` Milan Broz 2017-10-02 14:43 ` Mikulas Patocka 2017-10-03 6:27 ` Milan Broz 2017-10-03 12:05 ` Alasdair G Kergon 2017-10-03 18:08 ` Mike Snitzer 2017-10-03 19:09 ` Alasdair G Kergon 2017-10-03 20:08 ` Mikulas Patocka 2017-10-03 20:33 ` Milan Broz 2017-10-03 21:18 ` Mike Snitzer 2017-10-04 6:45 ` Milan Broz 2017-10-04 15:05 ` Mike Snitzer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.