On 7/25/22 03:42, Mariusz Tkaczyk wrote:

On Sat, 23 Jul 2022 14:37:11 +1000
"NeilBrown" [off-list ref] wrote:

quoted

On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote:

quoted

Hi Neil,

On Wed, 13 Jul 2022 13:48:11 +1000
"NeilBrown" [off-list ref] wrote:

....

quoted

quoted

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