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@xxxxxxx> wrote:
> >
> >> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote:
> >>> Hi Neil,
> >>>
> >>> On Wed, 13 Jul 2022 13:48:11 +1000
> >>> "NeilBrown" <neilb@xxxxxxx> 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
