linux-kernel - Re: [PATCH v1 2/5] PM: sleep: Remove unnecessary mutex lock when waiting on parent

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: <CAJZ5v0g9A1pZ5FjPAjdLY5ybNmefnBVVMJM7h3czW38p1fTfqQ@mail.gmail.com>
Date: Wed, 4 Dec 2024 13:21:57 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Saravana Kannan <saravanak@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Ingo Molnar <mingo@...hat.com>, 
	Peter Zijlstra <peterz@...radead.org>, Juri Lelli <juri.lelli@...hat.com>, 
	Vincent Guittot <vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>, 
	Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, Geert Uytterhoeven <geert@...ux-m68k.org>, Marek Vasut <marex@...x.de>, 
	Bird@...gle.com, Tim <Tim.Bird@...y.com>, kernel-team@...roid.com, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/5] PM: sleep: Remove unnecessary mutex lock when
 waiting on parent

On Tue, Dec 3, 2024 at 12:28 AM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Mon, Dec 2, 2024 at 1:15 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > On Mon, Dec 2, 2024 at 9:46 PM Saravana Kannan <saravanak@...gle.com> wrote:
> > >
> > > On Mon, Dec 2, 2024 at 12:16 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > >
> > > > On Mon, Dec 2, 2024 at 9:11 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > >
> > > > > Sorry for the delay.
> > > > >
> > > > > On Thu, Nov 14, 2024 at 11:09 PM Saravana Kannan <saravanak@...gle.com> wrote:
> > > > > >
> > > > > > Locking is not needed to do get_device(dev->parent). We either get a NULL
> > > > > > (if the parent was cleared) or the actual parent. Also, when a device is
> > > > > > deleted (device_del()) and removed from the dpm_list, its completion
> > > > > > variable is also complete_all()-ed. So, we don't have to worry about
> > > > > > waiting indefinitely on a deleted parent device.
> > > > >
> > > > > The device_pm_initialized(dev) check before get_device(dev->parent)
> > > > > doesn't make sense without the locking and that's the whole point of
> > > > > it.
> > > >
> > > > Hmm, not really.
> > > >
> > > > How is the parent prevented from going away in get_device() right
> > > > after the initial dev check without the locking?
> > >
> > > Not sure what you mean by "go away"? But get_device() is going to keep
> > > a non-zero refcount on the parent so that struct doesn't get freed.
> >
> > That's after it has returned.
> >
> > There is nothing to prevent dev from being freed in get_device()
> > itself before the kobject_get(&dev->kobj) call.
> >
> > So when get_device() is called, there needs to be an active ref on the
> > device already or something else to prevent the target device object
> > from being freed.
> >
> > In this particular case it is the lock along with the
> > device_pm_initialized(dev) check on the child.
>
> Ugh... my head hurts whenever I think about get_device(). Yeah, I
> think you are right.
>
> Hmm... I wanted to have this function be replaced by a call to a
> generic helper function dpm_for_each_superior() in the next patch. But
> that helper function could be called from places where the dpm_list
> lock is held. Also, I was planning on making it even more generic (not
> specific to dpm) in the future. So, depending on dpm_list lock didn't
> make sense.
>
> Any other ideas on how I could do this without grabbing the dpm_list mutex?

You don't need to replace the existing function with a new helper.

Just add the helper, use it going forward and drop the old function in
a separate patch when there are no more users of it.

> Actually, with the rewrite and at the end of this series, we don't
> have to worry about this race because each device's suspend/resume is
> only started after all the dependencies are done. So, if the parent
> deletes a child and itself, the child device's suspend wouldn't have
> been triggered at all.
>
> So, another option is to just squash the series and call it a day.

No, no.  This is complicated enough and the code is super-sensitive.

I think that you need to split the changes even more.

> Or add a comment to the commit text that this adds a race that's fixed by
> the time we get to the end of the series.

That would create a bisection trap, so not really.