linux-kernel - Re: [PATCH v3 2/2] of: overlay: Synchronize of_overlay

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: <20240304174933.7ad023f9@bootlin.com>
Date: Mon, 4 Mar 2024 17:49:33 +0100
From: Herve Codina <herve.codina@...tlin.com>
To: Rob Herring <robh@...nel.org>
Cc: Nuno Sá <noname.nuno@...il.com>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Frank Rowand <frowand.list@...il.com>, Lizhi Hou <lizhi.hou@....com>, Max
 Zhen <max.zhen@....com>, Sonal Santan <sonal.santan@....com>, Stefano
 Stabellini <stefano.stabellini@...inx.com>, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, Allan Nielsen <allan.nielsen@...rochip.com>,
 Horatiu Vultur <horatiu.vultur@...rochip.com>, Steen Hegelund
 <steen.hegelund@...rochip.com>, Luca Ceresoli <luca.ceresoli@...tlin.com>,
 Nuno Sa <nuno.sa@...log.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, stable@...r.kernel.org, Saravana Kannan
 <saravanak@...gle.com>
Subject: Re: [PATCH v3 2/2] of: overlay: Synchronize of_overlay_remove()
 with the devlink removals

Hi Rob,

On Mon, 4 Mar 2024 09:22:02 -0600
Rob Herring <robh@...nel.org> wrote:

..

> > > @@ -853,6 +854,14 @@ static void free_overlay_changeset(struct
> > > overlay_changeset *ovcs)
> > >  {
> > >  	int i;
> > >  
> > > +	/*
> > > +	 * Wait for any ongoing device link removals before removing some of
> > > +	 * nodes. Drop the global lock while waiting
> > > +	 */
> > > +	mutex_unlock(&of_mutex);
> > > +	device_link_wait_removal();
> > > +	mutex_lock(&of_mutex);  
> > 
> > I'm still not convinced we need to drop the lock. What happens if someone else
> > grabs the lock while we are in device_link_wait_removal()? Can we guarantee that
> > we can't screw things badly?  
> 
> It is also just ugly because it's the callers of 
> free_overlay_changeset() that hold the lock and now we're releasing it 
> behind their back.
> 
> As device_link_wait_removal() is called before we touch anything, can't 
> it be called before we take the lock? And do we need to call it if 
> applying the overlay fails?
> 

Indeed, having device_link_wait_removal() is not needed when applying the
overlay fails.

I can call device_link_wait_removal() from the caller of_overlay_remove()
but not before the lock is taken.
We need to call it between __of_changeset_revert_notify() and
free_overlay_changeset() and so, the lock is taken.

This lead to the following sequence:
--- 8< ---
int of_overlay_remove(int *ovcs_id)
{
	...
	mutex_lock(&of_mutex);
	...

	ret = __of_changeset_revert_notify(&ovcs->cset);
	...

	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
	...

	mutex_unlock(&of_mutex);
	device_link_wait_removal();
	mutex_lock(&of_mutex);

	free_overlay_changeset(ovcs);
	...
	mutex_unlock(&of_mutex);
	...
}
--- 8< ---

In this sequence, the question is:
Do we need to release the mutex lock while device_link_wait_removal() is
called ?

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com