Re: [PATCH] virt: acrn: fix invalid check past list iterator

[Jakob Koschel <jakobkoschel@gmail.com>]

> On 30. Mar 2022, at 09:57, Li Fei1 <fei1.li@intel.com> wrote:
> 
> On Sat, Mar 19, 2022 at 09:38:19PM +0100, Jakob Koschel wrote:
>> The condition retry == 0 is theoretically possible even if 'client'
>> does not point to a valid element because no break was hit.
>> 
>> To only execute the dev_warn if actually a break within the loop was
>> hit, a separate variable is used that is only set if it is ensured to
>> point to a valid client struct.
>> 
> Hi Koschel
> 
> Thanks for you to help us to try to improve the code. Maybe you don't get the point.
> The dev_warn should only been called when has_pending = true && retry == 0

Maybe I don't understand but looking isolated at this function I could see a way to call
the dev_warn() with has_pending = false && retry == 0.

> 		list_for_each_entry(client, &vm->ioreq_clients, list) {
> 			has_pending = has_pending_request(client);
> 			if (has_pending)
> 		}
> 		spin_unlock_bh(&vm->ioreq_clients_lock);

imagine has_pending == false && retry == 1 here, then client will not hold a valid list entry.

> 
> 		if (has_pending)
> 			schedule_timeout_interruptible(HZ / 100);
> 	} while (has_pending && --retry > 0);

since has_pending && --retry > 0 is no longer true the loop stops.

> 	if (retry == 0)
> 		dev_warn(acrn_dev.this_device,
> 			 "%s cannot flush pending request!\n", client->name);
client->name is accessed since retry == 0 now, but client is not a valid struct ending up
in a type confusion.

> 
> If retry > 0 and has_pending is true,  we would call schedule_timeout_interruptible
> to schedule out to wait all the pending I/O requests would been completed.
> 
> Thanks.

Again, I'm not sure if this is realistically possible. I'm trying to remove
any use of the list iterator after the loop to make such potentially issues detectable
at compile time instead of relying on certain (difficult to maintain) conditions to be met
to avoid the type confusion.

Thanks,
Jakob