[PATCH] imsm: Remove possibility for get_imsm_dev to return NULL

[PATCH] imsm: Remove possibility for get_imsm_dev to return NULL

[Mateusz Grzonka]

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@intel.com>
---
 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();
 }
 
 static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
 {
 	struct intel_dev *dv;
 
-	if (index >= super->anchor->num_raid_devs)
-		return NULL;
+	assert(index < super->anchor->num_raid_devs);
 	for (dv = super->devlist; dv; dv = dv->next)
 		if (dv->index == index)
 			return dv->dev;
-	return NULL;
+	pr_err("matching failed, index: %u\n", index);
+	abort();
 }
 
 static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
@@ -4358,8 +4357,7 @@ int check_mpb_migr_compatibility(struct intel_super *super)
 	for (i = 0; i < super->anchor->num_raid_devs; i++) {
 		struct imsm_dev *dev_iter = __get_imsm_dev(super->anchor, i);
 
-		if (dev_iter &&
-		    dev_iter->vol.migr_state == 1 &&
+		if (dev_iter->vol.migr_state == 1 &&
 		    dev_iter->vol.migr_type == MIGR_GEN_MIGR) {
 			/* This device is migrating */
 			map0 = get_imsm_map(dev_iter, MAP_0);
@@ -4508,8 +4506,6 @@ static void clear_hi(struct intel_super *super)
 	}
 	for (i = 0; i < mpb->num_raid_devs; ++i) {
 		struct imsm_dev *dev = get_imsm_dev(super, i);
-		if (!dev)
-			return;
 		for (n = 0; n < 2; ++n) {
 			struct imsm_map *map = get_imsm_map(dev, n);
 			if (!map)
@@ -5831,7 +5827,7 @@ static int add_to_super_imsm_volume(struct supertype *st, mdu_disk_info_t *dk,
 		struct imsm_dev *_dev = __get_imsm_dev(mpb, 0);
 
 		_disk = __get_imsm_disk(mpb, dl->index);
-		if (!_dev || !_disk) {
+		if (!_disk) {
 			pr_err("BUG mpb setup error\n");
 			return 1;
 		}
@@ -6168,10 +6164,10 @@ static int write_super_imsm(struct supertype *st, int doclose)
 	for (i = 0; i < mpb->num_raid_devs; i++) {
 		struct imsm_dev *dev = __get_imsm_dev(mpb, i);
 		struct imsm_dev *dev2 = get_imsm_dev(super, i);
-		if (dev && dev2) {
-			imsm_copy_dev(dev, dev2);
-			mpb_size += sizeof_imsm_dev(dev, 0);
-		}
+
+		imsm_copy_dev(dev, dev2);
+		mpb_size += sizeof_imsm_dev(dev, 0);
+
 		if (is_gen_migration(dev2))
 			clear_migration_record = 0;
 	}
@@ -9040,29 +9036,26 @@ static int imsm_rebuild_allowed(struct supertype *cont, int dev_idx, int failed)
 	__u8 state;
 
 	dev2 = get_imsm_dev(cont->sb, dev_idx);
-	if (dev2) {
-		state = imsm_check_degraded(cont->sb, dev2, failed, MAP_0);
-		if (state == IMSM_T_STATE_FAILED) {
-			map = get_imsm_map(dev2, MAP_0);
-			if (!map)
-				return 1;
-			for (slot = 0; slot < map->num_members; slot++) {
-				/*
-				 * Check if failed disks are deleted from intel
-				 * disk list or are marked to be deleted
-				 */
-				idx = get_imsm_disk_idx(dev2, slot, MAP_X);
-				idisk = get_imsm_dl_disk(cont->sb, idx);
-				/*
-				 * Do not rebuild the array if failed disks
-				 * from failed sub-array are not removed from
-				 * container.
-				 */
-				if (idisk &&
-				    is_failed(&idisk->disk) &&
-				    (idisk->action != DISK_REMOVE))
-					return 0;
-			}
+
+	state = imsm_check_degraded(cont->sb, dev2, failed, MAP_0);
+	if (state == IMSM_T_STATE_FAILED) {
+		map = get_imsm_map(dev2, MAP_0);
+		for (slot = 0; slot < map->num_members; slot++) {
+			/*
+			 * Check if failed disks are deleted from intel
+			 * disk list or are marked to be deleted
+			 */
+			idx = get_imsm_disk_idx(dev2, slot, MAP_X);
+			idisk = get_imsm_dl_disk(cont->sb, idx);
+			/*
+			 * Do not rebuild the array if failed disks
+			 * from failed sub-array are not removed from
+			 * container.
+			 */
+			if (idisk &&
+			    is_failed(&idisk->disk) &&
+			    (idisk->action != DISK_REMOVE))
+				return 0;
 		}
 	}
 	return 1;
@@ -10094,7 +10087,6 @@ static void imsm_process_update(struct supertype *st,
 		int victim = u->dev_idx;
 		struct active_array *a;
 		struct intel_dev **dp;
-		struct imsm_dev *dev;
 
 		/* sanity check that we are not affecting the uuid of
 		 * active arrays, or deleting an active array
@@ -10110,8 +10102,7 @@ static void imsm_process_update(struct supertype *st,
 		 * is active in the container, so checking
 		 * mpb->num_raid_devs is just extra paranoia
 		 */
-		dev = get_imsm_dev(super, victim);
-		if (a || !dev || mpb->num_raid_devs == 1) {
+		if (a || mpb->num_raid_devs == 1 || victim >= super->anchor->num_raid_devs) {
 			dprintf("failed to delete subarray-%d\n", victim);
 			break;
 		}
@@ -10145,7 +10136,7 @@ static void imsm_process_update(struct supertype *st,
 			if (a->info.container_member == target)
 				break;
 		dev = get_imsm_dev(super, u->dev_idx);
-		if (a || !dev || !check_name(super, name, 1)) {
+		if (a || !check_name(super, name, 1)) {
 			dprintf("failed to rename subarray-%d\n", target);
 			break;
 		}
@@ -10174,10 +10165,6 @@ static void imsm_process_update(struct supertype *st,
 		struct imsm_update_rwh_policy *u = (void *)update->buf;
 		int target = u->dev_idx;
 		struct imsm_dev *dev = get_imsm_dev(super, target);
-		if (!dev) {
-			dprintf("could not find subarray-%d\n", target);
-			break;
-		}
 
 		if (dev->rwh_policy != u->new_policy) {
 			dev->rwh_policy = u->new_policy;
@@ -11411,8 +11398,10 @@ static int imsm_create_metadata_update_for_migration(
 {
 	struct intel_super *super = st->sb;
 	int update_memory_size;
+	int current_chunk_size;
 	struct imsm_update_reshape_migration *u;
-	struct imsm_dev *dev;
+	struct imsm_dev *dev = get_imsm_dev(super, super->current_vol);
+	struct imsm_map *map = get_imsm_map(dev, MAP_0);
 	int previous_level = -1;
 
 	dprintf("(enter) New Level = %i\n", geo->level);
@@ -11429,23 +11418,15 @@ static int imsm_create_metadata_update_for_migration(
 	u->new_disks[0] = -1;
 	u->new_chunksize = -1;
 
-	dev = get_imsm_dev(super, u->subdev);
-	if (dev) {
-		struct imsm_map *map;
+	current_chunk_size = __le16_to_cpu(map->blocks_per_strip) / 2;
 
-		map = get_imsm_map(dev, MAP_0);
-		if (map) {
-			int current_chunk_size =
-				__le16_to_cpu(map->blocks_per_strip) / 2;
-
-			if (geo->chunksize != current_chunk_size) {
-				u->new_chunksize = geo->chunksize / 1024;
-				dprintf("imsm: chunk size change from %i to %i\n",
-					current_chunk_size, u->new_chunksize);
-			}
-			previous_level = map->raid_level;
-		}
+	if (geo->chunksize != current_chunk_size) {
+		u->new_chunksize = geo->chunksize / 1024;
+		dprintf("imsm: chunk size change from %i to %i\n",
+			current_chunk_size, u->new_chunksize);
 	}
+	previous_level = map->raid_level;
+
 	if (geo->level == 5 && previous_level == 0) {
 		struct mdinfo *spares = NULL;
 
@@ -12534,9 +12515,6 @@ static int validate_internal_bitmap_imsm(struct supertype *st)
 	unsigned long long offset;
 	struct dl *d;
 
-	if (!dev)
-		return -1;
-
 	if (dev->rwh_policy != RWH_BITMAP)
 		return 0;
 
@@ -12582,16 +12560,8 @@ static int add_internal_bitmap_imsm(struct supertype *st, int *chunkp,
 		return -1;
 
 	dev = get_imsm_dev(super, vol_idx);
-
-	if (!dev) {
-		dprintf("cannot find the device for volume index %d\n",
-			vol_idx);
-		return -1;
-	}
 	dev->rwh_policy = RWH_BITMAP;
-
 	*chunkp = calculate_bitmap_chunksize(st, dev);
-
 	return 0;
 }
 
-- 
2.26.2

Re: [PATCH] imsm: Remove possibility for get_imsm_dev to return NULL

[Jes Sorensen]

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@intel.com>
> ---
>  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.

Thanks,
Jes

Re: [PATCH] imsm: Remove possibility for get_imsm_dev to return NULL

[Tkaczyk, Mariusz]

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@intel.com>
>> ---
>>   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.

Thanks,
Mariusz

Re: [PATCH] imsm: Remove possibility for get_imsm_dev to return NULL

[Jes Sorensen]

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@intel.com>
>>> ---
>>>   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