linux-kernel - Re: [RFC PATCH] pinmux: Use sequential access to access desc->pinmux data

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] [day] [month] [year] [list]

Message-ID: <CACRpkdZxu34btYm-ibkgEqWfau97DjuKGib=-xd-WYL2PN_KYA@mail.gmail.com>
Date: Tue, 24 Sep 2024 09:42:58 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Mukesh Ojha <quic_mojha@...cinc.com>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] pinmux: Use sequential access to access desc->pinmux data

On Thu, Sep 12, 2024 at 4:25 PM Mukesh Ojha <quic_mojha@...cinc.com> wrote:

> When two client of the same gpio call pinctrl_select_state() for the
> same functionality, we are seeing NULL pointer issue while accessing
> desc->mux_owner.

Uh-oh it looks like a very real issue, weird that we didn't run into
it earlier.

I guess we were not parallelizing probe so much in the past so it
didn't happen for that reason.

>         /* Set owner */
>         pindesc->pctldev = pctldev;
> +#ifdef CONFIG_PINMUX
> +       spin_lock_init(&pindesc->lock);
> +#endif

Can we rename it "mux_lock" so it is clear what it is locking?

> @@ -115,6 +115,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
>         struct pin_desc *desc;
>         const struct pinmux_ops *ops = pctldev->desc->pmxops;
>         int status = -EINVAL;
> +       unsigned long flags;
>
>         desc = pin_desc_get(pctldev, pin);
>         if (desc == NULL) {
> @@ -127,6 +128,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
>         dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
>                 pin, desc->name, owner);
>
> +       spin_lock_irqsave(&desc->lock, flags);

Could you please rewrite all of these using scoped guards as that
avoids a lot of possible bugs?

#include <linux/cleanup.h>

guard(spinlock_irqsave)(&desc->mux_lock);

This means the lock will be released when you exit the
function .

tighter locks around a block of code are possible with:
scoped_guard(spinlock_irqsave, &desc->mux_lock) { ... }

It also removes the need to define a flags variable.

Yours,
Linus Walleij