Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 1 Apr 2022 09:15:33 +0800
From:   Li Fei1 <fei1.li@intel.com>
To:     Jakob Koschel <jakobkoschel@gmail.com>
Cc:     linux-kernel@vger.kernel.org, rppt@kernel.org,
        bjohannesmeyer@gmail.com, c.giuffrida@vu.nl, h.j.bos@vu.nl,
        fei1.li@intel.com
Subject: Re: [PATCH] virt: acrn: fix invalid check past list iterator
Message-ID: <20220401011533.GA29696@louislifei-OptiPlex-7050>
References: <20220319203819.2559993-1-jakobkoschel@gmail.com>
 <20220330075742.GA22544@louislifei-OptiPlex-7050>
 <6E68C33F-9CBB-418C-A11D-2AD863C0B19A@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6E68C33F-9CBB-418C-A11D-2AD863C0B19A@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Mar 31, 2022 at 01:20:50PM +0200, Jakob Koschel wrote:
> 
> > 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.
Yes, even has_pending = true && retry == 0 at the beginning, we could not make sure
has_pending is true after schedule_timeout_interruptible and we even didn't check
there're other pending client on the ioreq_clients (because we can't make sure
when we dev_warn this clent is still pending). So we just use dev_warn not higher log level.
> 
> > 		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.
What do you mean "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
You may think we still in the loop (could we ?), even that we didn't write the list iterator then.
> at compile time instead of relying on certain (difficult to maintain) conditions to be met
> to avoid the type confusion.
> 
> Thanks,
> Jakob
>