linux-kernel - Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAOesGMgq5HRz+pDFQtED5GPN0RUrON=_k_y4YHHzKrMkHnVk4A@mail.gmail.com>
Date:	Sun, 24 Jun 2012 16:54:55 -0700
From:	Olof Johansson <olof@...om.net>
To:	Bryan Freed <bfreed@...omium.org>
Cc:	spi-devel-general@...ts.sourceforge.net, grant.likely@...retlab.ca,
	linux-kernel@...r.kernel.org, LinusW <linus.walleij@...aro.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Subject: Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.

Hi,

(Adding Linus Walleij and Mark Brown since they were the ones doing
the queue changes).

On Fri, Jun 22, 2012 at 4:53 PM, Bryan Freed <bfreed@...omium.org> wrote:
> spi_pump_messages() calls into a controller driver with
> unprepare_transfer_hardware() which is documented as "This may sleep".
> As in the prepare_transfer_hardware() call below, we should release the
> queue_lock spinlock before making the call.
> Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
> then release it to call unprepare_transfer_hardware().
>
> Signed-off-by: Bryan Freed <bfreed@...omium.org>
>
> ---
>  drivers/spi/spi.c |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index fc0da39..f7f9df9 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -532,17 +532,16 @@ static void spi_pump_messages(struct kthread_work *work)
>        /* Lock queue and check for queue work */
>        spin_lock_irqsave(&master->queue_lock, flags);
>        if (list_empty(&master->queue) || !master->running) {
> -               if (master->busy && master->unprepare_transfer_hardware) {
> -                       ret = master->unprepare_transfer_hardware(master);
> -                       if (ret) {
> -                               spin_unlock_irqrestore(&master->queue_lock, flags);
> -                               dev_err(&master->dev,
> -                                       "failed to unprepare transfer hardware\n");
> -                               return;
> -                       }
> +               if (!master->busy) {
> +                       spin_unlock_irqrestore(&master->queue_lock, flags);
> +                       return;
>                }
>                master->busy = false;
>                spin_unlock_irqrestore(&master->queue_lock, flags);
> +               if (master->unprepare_transfer_hardware &&
> +                   master->unprepare_transfer_hardware(master))
> +                       dev_err(&master->dev,
> +                               "failed to unprepare transfer hardware\n");


I'm not sure this is safe to do. The lock is dropped for the prepare
side, but in that case we can be sure that the above code can't come
in and unprepare at the same time since there is per definition a
request on the queue at that time.

On the other hand, between the lock drop and the call to unprepare
above, another code path can come in and queue up a request and either
not do prepare properly, or there will be a prepare that races with
the unprepare.

It might make more sense to use a workqueue here and schedule a
unprepare call that way instead (and cancelling appropriately before
the prepare call if needed). I'm open for other suggestions as well.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/