linux-kernel - Re: [PATCH 1/1] driver core: Fix unbalance probe_count in really

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: <CAMuHMdV3sNojXqaxLmVjK3ziGugd3cWEKfLXikDwvaOmocTCzQ@mail.gmail.com>
Date:   Wed, 3 Jun 2020 09:13:12 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>, peter_hong@...tek.com.tw,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH 1/1] driver core: Fix unbalance probe_count in really_probe()

Hi Ji-Ze,

On Wed, Jun 3, 2020 at 8:45 AM Ji-Ze Hong (Peter Hong) <hpeter@...il.com> wrote:
> In previous patch, using return -EBUSY in really_probe() instead WARN_ON()
> only. The following is the partial code.
>
>         ...
>         atomic_inc(&probe_count);
>         pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
>                  drv->bus->name, __func__, drv->name, dev_name(dev));
>         if (!list_empty(&dev->devres_head)) {
>                 dev_crit(dev, "Resources present before probing\n");
>                 return -EBUSY;
>         }
>         ...
>
> When the devres_head is not empty, this code will return -EBUSY to prevent
> resource conflict, but it forgot to balance probe_count. We can move the
> increasement code below the resource checking.
>
>         ...
>         pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
>                  drv->bus->name, __func__, drv->name, dev_name(dev));
>         if (!list_empty(&dev->devres_head)) {
>                 dev_crit(dev, "Resources present before probing\n");
>                 return -EBUSY;
>         }
>         atomic_inc(&probe_count);

If devres_head is not empty, you have a serious problem on your system,
as those resources may be in an unknown state (e.g. freed but still in
use).  While I had missed the probe_count imbalance when implementing
the original change, it may actually be safer to not decrease
probe_count, to prevent further probes from happening.  But I guess it
doesn't matter: if you get here, your system is in a bad state anyway.

> The original code will cause lots motherboard freeze in reboot/shutdown
> with systemd message "Reached target Reboot" or "Reached target Shutdown"
> with serial8250 platform driver. e.g. AOPEN DE6200. The conflict boot
> dmesg below:
>
>         Serial: 8250/16550 driver, 32 ports, IRQ sharing enabled
>         00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 921600) is a 16550A
>         00:04: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 921600) is a 16550A
>         00:05: ttyS2 at I/O 0x3e8 (irq = 5, base_baud = 921600) is a 16550A
>         serial8250: ttyS3 at I/O 0x2e8 (irq = 3, base_baud = 921600) is a 16550A
>
> Reboot/Shutdown will freeze in wait_for_device_probe(), message as
> following:
>         INFQ: task systemd-shutdown: 1 blocked for more than 120 seconds.

Now, how did you get to this state, i.e. which driver triggered the
"Resources present before probing" message? Because that is the root
issue that must be fixed, and the probe_count imbalance is IMHO just a
red herring.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds