Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\))
Subject: Re: [PATCH] virt: acrn: fix invalid check past list iterator
From:   Jakob Koschel <jakobkoschel@gmail.com>
In-Reply-To: <20220330075742.GA22544@louislifei-OptiPlex-7050>
Date:   Thu, 31 Mar 2022 13:20:50 +0200
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>, h.j.bos@vu.nl
Content-Transfer-Encoding: quoted-printable
Message-Id: <6E68C33F-9CBB-418C-A11D-2AD863C0B19A@gmail.com>
References: <20220319203819.2559993-1-jakobkoschel@gmail.com>
 <20220330075742.GA22544@louislifei-OptiPlex-7050>
To:     Li Fei1 <fei1.li@intel.com>
Precedence: bulk


> On 30. Mar 2022, at 09:57, Li Fei1 <fei1.li@intel.com> wrote:
>=20
> On Sat, Mar 19, 2022 at 09:38:19PM +0100, Jakob Koschel wrote:
>> The condition retry =3D=3D 0 is theoretically possible even if =
'client'
>> does not point to a valid element because no break was hit.
>>=20
>> 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.
>>=20
> Hi Koschel
>=20
> 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 =3D true && =
retry =3D=3D 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 =3D false && retry =3D=3D 0.

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

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

>=20
> 		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 =3D=3D 0)
> 		dev_warn(acrn_dev.this_device,
> 			 "%s cannot flush pending request!\n", =
client->name);
client->name is accessed since retry =3D=3D 0 now, but client is not a =
valid struct ending up
in a type confusion.

>=20
> 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.
>=20
> 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