On 10/14/21 3:32 AM, Tkaczyk, Mariusz wrote:
> Hi Jes,
>
> On 08.10.2021 18:05, Jes Sorensen wrote:
>> On 9/20/21 7:10 AM, Mateusz Grzonka wrote:
>>> Returning NULL from get_imsm_dev or __get_imsm_dev will cause segfault.
>>> Guarantee that it never happens.
>>>
>>> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@xxxxxxxxx>
>>> ---
>>> super-intel.c | 122 +++++++++++++++++++-------------------------------
>>> 1 file changed, 46 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/super-intel.c b/super-intel.c
>>> index 83ddc000..2c3df58a 100644
>>> --- a/super-intel.c
>>> +++ b/super-intel.c
>>> @@ -858,30 +858,29 @@ static struct imsm_dev *__get_imsm_dev(struct
>>> imsm_super *mpb, __u8 index)
>>> void *_mpb = mpb;
>>> if (index >= mpb->num_raid_devs)
>>> - return NULL;
>>> + goto error;
>>> /* devices start after all disks */
>>> offset = ((void *) &mpb->disk[mpb->num_disks]) - _mpb;
>>> - for (i = 0; i <= index; i++)
>>> + for (i = 0; i <= index; i++, offset += sizeof_imsm_dev(_mpb +
>>> offset, 0))
>>> if (i == index)
>>> return _mpb + offset;
>>> - else
>>> - offset += sizeof_imsm_dev(_mpb + offset, 0);
>>> -
>>> - return NULL;
>>> +error:
>>> + pr_err("matching failed, index: %u\n", index);
>>> + abort();
>>> }
>>
>> Can we please fix the error handling properly instead of throwing in
>> abort() and assert() to avoid it. That's really sloppy in my book.
>
> This situation is unexpected and shall never happen. I don't see any
> advantage of handling it. Abort() is the most elegant solution.
> If it happens, it shall fail loudly. Verifying that we don't get
> NULL in each place where it is called is harmful and unnecessary.
>
> Passing wrong index means that code is broken and we definitely cannot
> continue.
If we are to fail with abort() at a bare minimum add some comments in
the code explaining why it's done.
Thanks,
Jes
