linux-hardening - Re: [PATCH v4 000/115] pwm: Fix lifetime issues for pwm

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

Date: Tue, 12 Dec 2023 22:05:09 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Thierry Reding <thierry.reding@...il.com>
Cc: linux-doc@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	platform-driver-x86@...r.kernel.org,
	linux-hardening@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-leds@...r.kernel.org, chrome-platform@...ts.linux.dev,
	linux-samsung-soc@...r.kernel.org,
	Bartosz Golaszewski <brgl@...ev.pl>, linux-staging@...ts.linux.dev,
	linux-rockchip@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	linux-pwm@...r.kernel.org, greybus-dev@...ts.linaro.org,
	linux-mediatek@...ts.infradead.org,
	linux-rpi-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-mips@...r.kernel.org,
	asahi@...ts.linux.dev, kernel@...gutronix.de
Subject: Re: [PATCH v4 000/115] pwm: Fix lifetime issues for pwm_chips

Hello Thierry,

On Mon, Dec 11, 2023 at 12:33:04PM +0100, Thierry Reding wrote:
> On Fri, Dec 08, 2023 at 07:50:33PM +0100, Uwe Kleine-König wrote:
> > You don't need to touch all drivers because you didn't change struct
> > pwm_chip::dev yet. (If you really want, you don't need to change that,
> > but then you have some duplication as chip->dev holds the same value as
> > priv->dev.parent in the end.)
> 
> I don't think that's a problem. These are for two logically separate
> things, after all.

How are they different? I'd say one is the initializer for the other and
(ideally) unused after that. With that interpretation they are indeed
different, but then it's ugly that the initializer keeps staying around.

> Duplication can also sometimes be useful to simplify
> things. There are plently of cases where we use local variables for the
> same reason.

local variables go away though after the respective function is left.
chip->dev and its copy priv->dev.parent stay around for the full
lifetime of the chip.

> > > @@ -58,23 +60,24 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name)
> > >  
> > >  static int pwm_device_request(struct pwm_device *pwm, const char *label)
> > >  {
> > > +	struct pwm_chip *chip = pwm->priv->chip;
> > 
> > With my approach getting the chip of a struct pwm_device is only one
> > pointer dereference away. You need two.
> 
> None of the functions here are called very often, so even if this isn't
> optimized away it would hardly matter.

I'd say pwm_apply_state() at least matters. Also I think that making a
slow path quicker is a good thing. 

I wonder how we'll converge to an approach that can go into the
mainline given that we both have our strong opinions.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)