linux-kernel - Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE-0n52REGuVrqG8LUdWzjgqAun7uvDFPJeSkjX02OxD=fUhqg@mail.gmail.com>
Date:   Wed, 6 Sep 2023 15:14:54 -0500
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        platform-driver-x86@...r.kernel.org,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Prashant Malani <pmalani@...omium.org>
Subject: Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after
 timeout in busy_loop()

Quoting Andy Shevchenko (2023-09-06 13:04:54)
> On Wed, Sep 06, 2023 at 11:09:41AM -0700, Stephen Boyd wrote:
> > It's possible for the polling loop in busy_loop() to get scheduled away
> > for a long time.
> >
> >   status = ipc_read_status(scu); // status = IPC_STATUS_BUSY
> >   <long time scheduled away>
> >   if (!(status & IPC_STATUS_BUSY))
> >
> > If this happens, then the status bit could change while the task is
> > scheduled away and this function would never read the status again after
> > timing out. Instead, the function will return -ETIMEDOUT when it's
> > possible that scheduling didn't work out and the status bit was cleared.
> > Bit polling code should always check the bit being polled one more time
> > after the timeout in case this happens.
> >
> > Fix this by reading the status once more after the while loop breaks.
>
> ...
>
> >  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
> >  {
> >       unsigned long end = jiffies + IPC_TIMEOUT;
> > +     u32 status;
> >
> >       do {
> > -             u32 status;
> > -
> >               status = ipc_read_status(scu);
> >               if (!(status & IPC_STATUS_BUSY))
>
> > -                     return (status & IPC_STATUS_ERR) ? -EIO : 0;
> > +                     goto not_busy;
>
> Wouldn't simple 'break' suffice here?

Yes, at the cost of reading the status again when it isn't busy, or
checking the busy bit after the loop breaks out and reading it once
again when it is busy. I suppose the compiler would figure that out and
optimize so that break would simply goto the return statement.

The code could look like this without a goto.

	do {
		status = ipc_read_status(scu);
		if (!(status & IPC_STATUS_BUSY))
			break;
	} while (time_before(jiffies, end));

	if (status & IPC_STATUS_BUSY)
		status = ipc_read_status(scu);

	if (status & IPC_STATUS_BUSY)
		return -ETIMEDOUT;
	
	return (status & IPC_STATUS_ERR) ? -EIO : 0;