| Message ID | 165768409124.25184.3270769367375387242@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mdadm,v2] super1: report truncated device | expand |
Hi Neil, On Wed, 13 Jul 2022 13:48:11 +1000 "NeilBrown" <neilb@suse.de> wrote: > When the metadata is at the start of the device, it is possible that it > describes a device large than the one it is actually stored on. When > this happens, report it loudly in --examine. > > .... > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL > State : clean TRUNCATED DEVICE > .... State : clean TRUNCATED DEVICE is enough. "DEVICE TOO SMALL" seems to be redundant. > > Also report in --assemble so that the failure which the kernel will > report will be explained. Understand but you've added it in load_super1() so it affects all load_super() calls, is it indented? I assume yes but please confirm. > > mdadm: Device /dev/sdb is not large enough for data described in superblock > mdadm: no RAID superblock on /dev/sdb > mdadm: /dev/sdb has no superblock - assembly aborted > > Scenario can be demonstrated as follows: > > mdadm: Note: this array has metadata at the start and > may not be suitable as a boot device. If you plan to > store '/boot' on this device please ensure that > your boot-loader understands md/v1.x metadata, or use > --metadata=0.90 > mdadm: Defaulting to version 1.2 metadata > mdadm: array /dev/md/test started. > mdadm: stopped /dev/md/test > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL > State : clean TRUNCATED DEVICE > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL > State : clean TRUNCATED DEVICE > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > super1.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/super1.c b/super1.c > index 71af860c0e3e..4d8dba8a5a44 100644 > --- a/super1.c > +++ b/super1.c > @@ -406,12 +406,18 @@ static void examine_super1(struct supertype *st, char > *homehost) > st->ss->getinfo_super(st, &info, NULL); > if (info.space_after != 1 && > - !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) > - printf(" Unused Space : before=%llu sectors, after=%llu > sectors\n", > - info.space_before, info.space_after); > - > - printf(" State : %s\n", > - (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean"); > + !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) { > + printf(" Unused Space : before=%llu sectors, ", > + info.space_before); > + if (info.space_after < INT64_MAX) > + printf("after=%llu sectors\n", info.space_after); > + else > + printf("after=-%llu sectors DEVICE TOO SMALL\n", > + UINT64_MAX - info.space_after); As above, for me this else here is not necessary. > + } > + printf(" State : %s%s\n", > + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean", > + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : ""); Could you use standard if instruction to make the code more readable? We are avoiding ternary operators if possible now. > printf(" Device UUID : "); > for (i=0; i<16; i++) { > if ((i&3)==0 && i != 0) > @@ -2206,6 +2212,7 @@ static int load_super1(struct supertype *st, int fd, > char *devname) tst.ss = &super1; > for (tst.minor_version = 0; tst.minor_version <= 2; > tst.minor_version++) { > + tst.ignore_hw_compat = st->ignore_hw_compat; > switch(load_super1(&tst, fd, devname)) { > case 0: super = tst.sb; > if (bestvers == -1 || > @@ -2312,7 +2319,6 @@ static int load_super1(struct supertype *st, int fd, > char *devname) free(super); > return 2; > } > - st->sb = super; > > bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE); > > @@ -2322,6 +2328,20 @@ static int load_super1(struct supertype *st, int fd, > char *devname) if (st->data_offset == INVALID_SECTORS) > st->data_offset = __le64_to_cpu(super->data_offset); > > + if (st->minor_version >= 1 && > + st->ignore_hw_compat == 0 && > + (__le64_to_cpu(super->data_offset) + > + __le64_to_cpu(super->size) > dsize || > + __le64_to_cpu(super->data_offset) + > + __le64_to_cpu(super->data_size) > dsize)) { > + if (devname) > + pr_err("Device %s is not large enough for data > described in superblock\n", > + devname); why not just: if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) > dsize) from my understanding, only this check matters. Thanks, Mariusz
Dear Mariusz, Am 21.07.22 um 10:19 schrieb Mariusz Tkaczyk: > On Wed, 13 Jul 2022 13:48:11 +1000 NeilBrown wrote: […] >> + } >> + printf(" State : %s%s\n", >> + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean", >> + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : ""); > > Could you use standard if instruction to make the code more readable? We are > avoiding ternary operators if possible now. That’s news to me. Where is that documented? If find the operator quite useful in situations like this. Kind regards, Paul
On Thu, 21 Jul 2022 18:21:46 +0200 Paul Menzel <pmenzel@molgen.mpg.de> wrote: > Dear Mariusz, > > > Am 21.07.22 um 10:19 schrieb Mariusz Tkaczyk: > > > On Wed, 13 Jul 2022 13:48:11 +1000 NeilBrown wrote: > > […] > > >> + } > >> + printf(" State : %s%s\n", > >> + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean", > >> + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : ""); > > > > Could you use standard if instruction to make the code more readable? We are > > avoiding ternary operators if possible now. > > That’s news to me. Where is that documented? If find the operator quite > useful in situations like this. > > Hi Paul, It was Jes's preference, however I don't remember exactly when and where he pointed that (and I cannot find it now). To clarify - I meant inline\ternary if only. Jes, could you look? As you said, in this case ternary is useful, so I give it to Neil to decide if it can be easily replaced. If not- I'm fine with current approach. Thanks, Mariusz
On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote: > Hi Neil, > > On Wed, 13 Jul 2022 13:48:11 +1000 > "NeilBrown" <neilb@suse.de> wrote: > > > When the metadata is at the start of the device, it is possible that it > > describes a device large than the one it is actually stored on. When > > this happens, report it loudly in --examine. > > > > .... > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL > > State : clean TRUNCATED DEVICE > > .... > > State : clean TRUNCATED DEVICE is enough. "DEVICE TOO SMALL" seems to be > redundant. I needed to change the "Unused Space" line because before the patch the "after=" value is close to 2^64. I needed to make it negative. But having a negative value there is strange so I thought it would be good to highlight it and explain why. > > > > Also report in --assemble so that the failure which the kernel will > > report will be explained. > > Understand but you've added it in load_super1() so it affects all load_super() > calls, is it indented? I assume yes but please confirm. Yes, it is intended for all calls to ->load_super() on v1 metadata. The test is gated on ->ignore_hw_compat so that it does still look like v1.x metadata (so --examine can report on it), but an error results for any attempt to use the metadata in an active array. ->ignore_hw_compat isn't a perfect fit for the concept, but it is a perfect fit for the desired behaviour. Maybe we should rethink the name for that field. > > > > mdadm: Device /dev/sdb is not large enough for data described in superblock > > mdadm: no RAID superblock on /dev/sdb > > mdadm: /dev/sdb has no superblock - assembly aborted > > > > Scenario can be demonstrated as follows: > > > > mdadm: Note: this array has metadata at the start and > > may not be suitable as a boot device. If you plan to > > store '/boot' on this device please ensure that > > your boot-loader understands md/v1.x metadata, or use > > --metadata=0.90 > > mdadm: Defaulting to version 1.2 metadata > > mdadm: array /dev/md/test started. > > mdadm: stopped /dev/md/test > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL > > State : clean TRUNCATED DEVICE > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL > > State : clean TRUNCATED DEVICE > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > super1.c | 34 +++++++++++++++++++++++++++------- > > 1 file changed, 27 insertions(+), 7 deletions(-) > > > > diff --git a/super1.c b/super1.c > > index 71af860c0e3e..4d8dba8a5a44 100644 > > --- a/super1.c > > +++ b/super1.c > > @@ -406,12 +406,18 @@ static void examine_super1(struct supertype *st, char > > *homehost) > > st->ss->getinfo_super(st, &info, NULL); > > if (info.space_after != 1 && > > - !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) > > - printf(" Unused Space : before=%llu sectors, after=%llu > > sectors\n", > > - info.space_before, info.space_after); > > - > > - printf(" State : %s\n", > > - (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean"); > > + !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) { > > + printf(" Unused Space : before=%llu sectors, ", > > + info.space_before); > > + if (info.space_after < INT64_MAX) > > + printf("after=%llu sectors\n", info.space_after); > > + else > > + printf("after=-%llu sectors DEVICE TOO SMALL\n", > > + UINT64_MAX - info.space_after); > As above, for me this else here is not necessary. The change to report a negative is necessary. > > > + } > > + printf(" State : %s%s\n", > > + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean", > > + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : ""); > > Could you use standard if instruction to make the code more readable? We are > avoiding ternary operators if possible now. I could. I don't want to. I think the code is quite readable. Putting a space before the first '?' would help, as might lining up the two '?'. > > > printf(" Device UUID : "); > > for (i=0; i<16; i++) { > > if ((i&3)==0 && i != 0) > > @@ -2206,6 +2212,7 @@ static int load_super1(struct supertype *st, int fd, > > char *devname) tst.ss = &super1; > > for (tst.minor_version = 0; tst.minor_version <= 2; > > tst.minor_version++) { > > + tst.ignore_hw_compat = st->ignore_hw_compat; > > switch(load_super1(&tst, fd, devname)) { > > case 0: super = tst.sb; > > if (bestvers == -1 || > > @@ -2312,7 +2319,6 @@ static int load_super1(struct supertype *st, int fd, > > char *devname) free(super); > > return 2; > > } > > - st->sb = super; > > > > bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE); > > > > @@ -2322,6 +2328,20 @@ static int load_super1(struct supertype *st, int fd, > > char *devname) if (st->data_offset == INVALID_SECTORS) > > st->data_offset = __le64_to_cpu(super->data_offset); > > > > + if (st->minor_version >= 1 && > > + st->ignore_hw_compat == 0 && > > + (__le64_to_cpu(super->data_offset) + > > + __le64_to_cpu(super->size) > dsize || > > + __le64_to_cpu(super->data_offset) + > > + __le64_to_cpu(super->data_size) > dsize)) { > > + if (devname) > > + pr_err("Device %s is not large enough for data > > described in superblock\n", > > + devname); > > why not just: > if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) > dsize) > from my understanding, only this check matters. It seemed safest to test both. I don't remember the difference between ->size and ->data_size. In getinfo_super1() we have if (info->array.level <= 0) data_size = __le64_to_cpu(sb->data_size); else data_size = __le64_to_cpu(sb->size); which suggests that either could be relevant. I guess ->size should always be less than ->data_size. But load_super1() doesn't check that, so it isn't safe to assume it. Thanks, NeilBrown > > Thanks, > Mariusz > >
On Sat, 23 Jul 2022 14:37:11 +1000 "NeilBrown" <neilb@suse.de> wrote: > On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote: > > Hi Neil, > > > > On Wed, 13 Jul 2022 13:48:11 +1000 > > "NeilBrown" <neilb@suse.de> wrote: > > > > > When the metadata is at the start of the device, it is possible that it > > > describes a device large than the one it is actually stored on. When > > > this happens, report it loudly in --examine. > > > > > > .... > > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO > > > SMALL State : clean TRUNCATED DEVICE > > > .... > > > > State : clean TRUNCATED DEVICE is enough. "DEVICE TOO SMALL" seems to be > > redundant. > > I needed to change the "Unused Space" line because before the patch the > "after=" value is close to 2^64. I needed to make it negative. But having > a negative value there is strange so I thought it would be good to > highlight it and explain why. Got it, thanks. > > > > > > > Also report in --assemble so that the failure which the kernel will > > > report will be explained. > > > > Understand but you've added it in load_super1() so it affects all > > load_super() calls, is it indented? I assume yes but please confirm. > > Yes, it is intended for all calls to ->load_super() on v1 metadata. > The test is gated on ->ignore_hw_compat so that it does still look like > v1.x metadata (so --examine can report on it), but an error results for > any attempt to use the metadata in an active array. > > ->ignore_hw_compat isn't a perfect fit for the concept, but it is a > perfect fit for the desired behaviour. Maybe we should rethink the name > for that field. > > > > > > > mdadm: Device /dev/sdb is not large enough for data described in > > > superblock mdadm: no RAID superblock on /dev/sdb > > > mdadm: /dev/sdb has no superblock - assembly aborted > > > > > > Scenario can be demonstrated as follows: > > > > > > mdadm: Note: this array has metadata at the start and > > > may not be suitable as a boot device. If you plan to > > > store '/boot' on this device please ensure that > > > your boot-loader understands md/v1.x metadata, or use > > > --metadata=0.90 > > > mdadm: Defaulting to version 1.2 metadata > > > mdadm: array /dev/md/test started. > > > mdadm: stopped /dev/md/test > > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO > > > SMALL State : clean TRUNCATED DEVICE > > > Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO > > > SMALL State : clean TRUNCATED DEVICE > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > super1.c | 34 +++++++++++++++++++++++++++------- > > > 1 file changed, 27 insertions(+), 7 deletions(-) > > > > > > diff --git a/super1.c b/super1.c > > > index 71af860c0e3e..4d8dba8a5a44 100644 > > > --- a/super1.c > > > +++ b/super1.c > > > @@ -406,12 +406,18 @@ static void examine_super1(struct supertype *st, > > > char *homehost) > > > st->ss->getinfo_super(st, &info, NULL); > > > if (info.space_after != 1 && > > > - !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) > > > - printf(" Unused Space : before=%llu sectors, after=%llu > > > sectors\n", > > > - info.space_before, info.space_after); > > > - > > > - printf(" State : %s\n", > > > - (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean"); > > > + !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) { > > > + printf(" Unused Space : before=%llu sectors, ", > > > + info.space_before); > > > + if (info.space_after < INT64_MAX) > > > + printf("after=%llu sectors\n", info.space_after); > > > + else > > > + printf("after=-%llu sectors DEVICE TOO SMALL\n", > > > + UINT64_MAX - info.space_after); > > As above, for me this else here is not necessary. > > The change to report a negative is necessary. > > > > > > + } > > > + printf(" State : %s%s\n", > > > + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean", Please add space before '?' and between and after ':' (same as below). > > > + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : ""); > > > > > > > Could you use standard if instruction to make the code more readable? We are > > avoiding ternary operators if possible now. > > I could. I don't want to. > I think the code is quite readable. Putting a space before the first > '?' would help, as might lining up the two '?'. Please fix formatting and I'm fine with that. In this case ternary if is reasonable. > > > > > > printf(" Device UUID : "); > > > for (i=0; i<16; i++) { > > > if ((i&3)==0 && i != 0) > > > @@ -2206,6 +2212,7 @@ static int load_super1(struct supertype *st, int fd, > > > char *devname) tst.ss = &super1; > > > for (tst.minor_version = 0; tst.minor_version <= 2; > > > tst.minor_version++) { > > > + tst.ignore_hw_compat = st->ignore_hw_compat; > > > switch(load_super1(&tst, fd, devname)) { > > > case 0: super = tst.sb; > > > if (bestvers == -1 || > > > @@ -2312,7 +2319,6 @@ static int load_super1(struct supertype *st, int fd, > > > char *devname) free(super); > > > return 2; > > > } > > > - st->sb = super; > > > > > > bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE); > > > > > > @@ -2322,6 +2328,20 @@ static int load_super1(struct supertype *st, int > > > fd, char *devname) if (st->data_offset == INVALID_SECTORS) > > > st->data_offset = __le64_to_cpu(super->data_offset); > > > > > > + if (st->minor_version >= 1 && > > > + st->ignore_hw_compat == 0 && > > > + (__le64_to_cpu(super->data_offset) + > > > + __le64_to_cpu(super->size) > dsize || > > > + __le64_to_cpu(super->data_offset) + > > > + __le64_to_cpu(super->data_size) > dsize)) { > > > + if (devname) > > > + pr_err("Device %s is not large enough for data > > > described in superblock\n", > > > + devname); > > > > why not just: > > if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) > > > dsize) from my understanding, only this check matters. > > It seemed safest to test both. I don't remember the difference between > ->size and ->data_size. In getinfo_super1() we have > > if (info->array.level <= 0) > data_size = __le64_to_cpu(sb->data_size); > else > data_size = __le64_to_cpu(sb->size); > > which suggests that either could be relevant. > I guess ->size should always be less than ->data_size. But > load_super1() doesn't check that, so it isn't safe to assume it. Honestly, I don't understand why but I didn't realize that you are checking two different fields (size and data_size). I focused on understanding what is going on here, and didn't catch difference in variables (because data_offset and data_size have similar prefix). For me, something like: unsigned long long _size; if (st->minor_version >= 1 && st->ignore_hw_compat == 0) _size= __le64_to_cpu(super->size); else _size= __le64_to_cpu(super->data_size); if (__le64_to_cpu(super->data_offset) + _size > dsize) {....} is more readable because I don't need to analyze complex if to get the difference. Also, I removed doubled __le64_to_cpu(super->data_offset). Could you refactor this part? Thanks, Mariusz
On 7/25/22 03:42, Mariusz Tkaczyk wrote: > On Sat, 23 Jul 2022 14:37:11 +1000 > "NeilBrown" <neilb@suse.de> wrote: > >> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote: >>> Hi Neil, >>> >>> On Wed, 13 Jul 2022 13:48:11 +1000 >>> "NeilBrown" <neilb@suse.de> wrote: .... >>> why not just: >>> if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) > >>> dsize) from my understanding, only this check matters. >> >> It seemed safest to test both. I don't remember the difference between >> ->size and ->data_size. In getinfo_super1() we have >> >> if (info->array.level <= 0) >> data_size = __le64_to_cpu(sb->data_size); >> else >> data_size = __le64_to_cpu(sb->size); >> >> which suggests that either could be relevant. >> I guess ->size should always be less than ->data_size. But >> load_super1() doesn't check that, so it isn't safe to assume it. > > Honestly, I don't understand why but I didn't realize that you are checking two > different fields (size and data_size). I focused on understanding what is going > on here, and didn't catch difference in variables (because data_offset and > data_size have similar prefix). > For me, something like: > > unsigned long long _size; > if (st->minor_version >= 1 && st->ignore_hw_compat == 0) > _size= __le64_to_cpu(super->size); > else > _size= __le64_to_cpu(super->data_size); > > if (__le64_to_cpu(super->data_offset) + _size > dsize) > {....} > > is more readable because I don't need to analyze complex if to get the > difference. Also, I removed doubled __le64_to_cpu(super->data_offset). > Could you refactor this part? What is the consensus on this discussion? I see Coly pulled this into his tree, but I don't see Mariusz's last concern being addressed. Thanks, Jes
On Thu, 25 Aug 2022, Jes Sorensen wrote: > On 7/25/22 03:42, Mariusz Tkaczyk wrote: > > On Sat, 23 Jul 2022 14:37:11 +1000 > > "NeilBrown" <neilb@suse.de> wrote: > > > >> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote: > >>> Hi Neil, > >>> > >>> On Wed, 13 Jul 2022 13:48:11 +1000 > >>> "NeilBrown" <neilb@suse.de> wrote: > .... > >>> why not just: > >>> if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) > > >>> dsize) from my understanding, only this check matters. > >> > >> It seemed safest to test both. I don't remember the difference between > >> ->size and ->data_size. In getinfo_super1() we have > >> > >> if (info->array.level <= 0) > >> data_size = __le64_to_cpu(sb->data_size); > >> else > >> data_size = __le64_to_cpu(sb->size); > >> > >> which suggests that either could be relevant. > >> I guess ->size should always be less than ->data_size. But > >> load_super1() doesn't check that, so it isn't safe to assume it. > > > > Honestly, I don't understand why but I didn't realize that you are checking two > > different fields (size and data_size). I focused on understanding what is going > > on here, and didn't catch difference in variables (because data_offset and > > data_size have similar prefix). > > For me, something like: > > > > unsigned long long _size; > > if (st->minor_version >= 1 && st->ignore_hw_compat == 0) > > _size= __le64_to_cpu(super->size); > > else > > _size= __le64_to_cpu(super->data_size); > > > > if (__le64_to_cpu(super->data_offset) + _size > dsize) > > {....} > > > > is more readable because I don't need to analyze complex if to get the > > difference. Also, I removed doubled __le64_to_cpu(super->data_offset). > > Could you refactor this part? > > What is the consensus on this discussion? I see Coly pulled this into > his tree, but I don't see Mariusz's last concern being addressed. I don't think we reached a consensus. I probably got distracted. I don't like that suggestion from Mariusz as it makes assumptions that I didn't want to make. I think it is safest to always test dsize against bother ->size and ->data_size without baking in assumptions about when either is meaningful. NeilBrown
On Thu, 25 Aug 2022 10:24:21 +1000 "NeilBrown" <neilb@suse.de> wrote: > On Thu, 25 Aug 2022, Jes Sorensen wrote: > > On 7/25/22 03:42, Mariusz Tkaczyk wrote: > > > On Sat, 23 Jul 2022 14:37:11 +1000 > > > "NeilBrown" <neilb@suse.de> wrote: > > > > > >> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote: > > >>> Hi Neil, > > >>> > > >>> On Wed, 13 Jul 2022 13:48:11 +1000 > > >>> "NeilBrown" <neilb@suse.de> wrote: > > .... > > >>> why not just: > > >>> if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) > > >>> > dsize) from my understanding, only this check matters. > > >> > > >> It seemed safest to test both. I don't remember the difference between > > >> ->size and ->data_size. In getinfo_super1() we have > > >> > > >> if (info->array.level <= 0) > > >> data_size = __le64_to_cpu(sb->data_size); > > >> else > > >> data_size = __le64_to_cpu(sb->size); > > >> > > >> which suggests that either could be relevant. > > >> I guess ->size should always be less than ->data_size. But > > >> load_super1() doesn't check that, so it isn't safe to assume it. > > > > > > Honestly, I don't understand why but I didn't realize that you are > > > checking two different fields (size and data_size). I focused on > > > understanding what is going on here, and didn't catch difference in > > > variables (because data_offset and data_size have similar prefix). > > > For me, something like: > > > > > > unsigned long long _size; > > > if (st->minor_version >= 1 && st->ignore_hw_compat == 0) > > > _size= __le64_to_cpu(super->size); > > > else > > > _size= __le64_to_cpu(super->data_size); > > > > > > if (__le64_to_cpu(super->data_offset) + _size > dsize) > > > {....} > > > > > > is more readable because I don't need to analyze complex if to get the > > > difference. Also, I removed doubled __le64_to_cpu(super->data_offset). > > > Could you refactor this part? > > > > What is the consensus on this discussion? I see Coly pulled this into > > his tree, but I don't see Mariusz's last concern being addressed. > > I don't think we reached a consensus. I probably got distracted. > I don't like that suggestion from Mariusz as it makes assumptions that I > didn't want to make. I think it is safest to always test dsize against > bother ->size and ->data_size without baking in assumptions about when > either is meaningful. > Hi Neil, It seems that I failed to understand it again. You are right, you approach is safer. Please fix stylistic issues then and I'm fine with the change. Sorry for confusing you, Mariusz
On 8/25/22 03:59, Mariusz Tkaczyk wrote: > On Thu, 25 Aug 2022 10:24:21 +1000 > "NeilBrown" <neilb@suse.de> wrote: >>> What is the consensus on this discussion? I see Coly pulled this into >>> his tree, but I don't see Mariusz's last concern being addressed. >> >> I don't think we reached a consensus. I probably got distracted. >> I don't like that suggestion from Mariusz as it makes assumptions that I >> didn't want to make. I think it is safest to always test dsize against >> bother ->size and ->data_size without baking in assumptions about when >> either is meaningful. No worries, distraction is my middle name these days :) > Hi Neil, > It seems that I failed to understand it again. You are right, you approach is > safer. Please fix stylistic issues then and I'm fine with the change. Thanks Mariusz Jes
diff --git a/super1.c b/super1.c index 71af860c0e3e..4d8dba8a5a44 100644 --- a/super1.c +++ b/super1.c @@ -406,12 +406,18 @@ static void examine_super1(struct supertype *st, char *homehost) st->ss->getinfo_super(st, &info, NULL); if (info.space_after != 1 && - !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) - printf(" Unused Space : before=%llu sectors, after=%llu sectors\n", - info.space_before, info.space_after); - - printf(" State : %s\n", - (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean"); + !(__le32_to_cpu(sb->feature_map) & MD_FEATURE_NEW_OFFSET)) { + printf(" Unused Space : before=%llu sectors, ", + info.space_before); + if (info.space_after < INT64_MAX) + printf("after=%llu sectors\n", info.space_after); + else + printf("after=-%llu sectors DEVICE TOO SMALL\n", + UINT64_MAX - info.space_after); + } + printf(" State : %s%s\n", + (__le64_to_cpu(sb->resync_offset)+1)? "active":"clean", + info.space_after > INT64_MAX ? " TRUNCATED DEVICE" : ""); printf(" Device UUID : "); for (i=0; i<16; i++) { if ((i&3)==0 && i != 0) @@ -2206,6 +2212,7 @@ static int load_super1(struct supertype *st, int fd, char *devname) tst.ss = &super1; for (tst.minor_version = 0; tst.minor_version <= 2; tst.minor_version++) { + tst.ignore_hw_compat = st->ignore_hw_compat; switch(load_super1(&tst, fd, devname)) { case 0: super = tst.sb; if (bestvers == -1 || @@ -2312,7 +2319,6 @@ static int load_super1(struct supertype *st, int fd, char *devname) free(super); return 2; } - st->sb = super; bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE); @@ -2322,6 +2328,20 @@ static int load_super1(struct supertype *st, int fd, char *devname) if (st->data_offset == INVALID_SECTORS) st->data_offset = __le64_to_cpu(super->data_offset); + if (st->minor_version >= 1 && + st->ignore_hw_compat == 0 && + (__le64_to_cpu(super->data_offset) + + __le64_to_cpu(super->size) > dsize || + __le64_to_cpu(super->data_offset) + + __le64_to_cpu(super->data_size) > dsize)) { + if (devname) + pr_err("Device %s is not large enough for data described in superblock\n", + devname); + free(super); + return 2; + } + st->sb = super; + /* Now check on the bitmap superblock */ if ((__le32_to_cpu(super->feature_map)&MD_FEATURE_BITMAP_OFFSET) == 0) return 0;
When the metadata is at the start of the device, it is possible that it describes a device large than the one it is actually stored on. When this happens, report it loudly in --examine. .... Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL State : clean TRUNCATED DEVICE .... Also report in --assemble so that the failure which the kernel will report will be explained. mdadm: Device /dev/sdb is not large enough for data described in superblock mdadm: no RAID superblock on /dev/sdb mdadm: /dev/sdb has no superblock - assembly aborted Scenario can be demonstrated as follows: mdadm: Note: this array has metadata at the start and may not be suitable as a boot device. If you plan to store '/boot' on this device please ensure that your boot-loader understands md/v1.x metadata, or use --metadata=0.90 mdadm: Defaulting to version 1.2 metadata mdadm: array /dev/md/test started. mdadm: stopped /dev/md/test Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL State : clean TRUNCATED DEVICE Unused Space : before=1968 sectors, after=-2047 sectors DEVICE TOO SMALL State : clean TRUNCATED DEVICE Signed-off-by: NeilBrown <neilb@suse.de> --- super1.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)