linux-kernel - Re: [PATCH 5/6] soudnwire: master: protect concurrecnt check for bus->md

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]

Date:   Thu, 20 Apr 2023 11:42:18 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Vinod Koul <vkoul@...nel.org>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Cc:     Patrick Lai <quic_plai@...cinc.com>
Subject: Re: [PATCH 5/6] soudnwire: master: protect concurrecnt check for
 bus->md

typos in commit title...

On 4/20/23 05:16, Krzysztof Kozlowski wrote:
> The Soundwire master controllers might want to check for bus->md

Apologies for being pedantic but 'manager' and 'controller' are
different concepts in SoundWire, see DisCo spec.
It's not a 1:1 mapping, a controller can rely on M managers

> initialization to avoid race between early interrupt and finish of
> sdw_bus_master_add()/sdw_master_device_add().  Such early interrupt can
> happen if Soundwire devices are not powered off during their probe.
> 
> Add a store release barrier, so the Soundwire controllers can safely
> check it in concurrent (e.g. in interrupt) way.

Can you elaborate on the race condition? I am not following what breaks,
and what entity generates the 'early interrupt'.

I am specifically concerned about adding this in common code without any
matching smp_load_acquire() - which is only added in the following patch
for the Qualcomm manager only, but not added for Intel/AMD managers. Is
this not a problem?

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> 
> ---
> 
> Cc: Patrick Lai <quic_plai@...cinc.com>
> ---
>  drivers/soundwire/master.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> index 9b05c9e25ebe..d5bf13e7e602 100644
> --- a/drivers/soundwire/master.c
> +++ b/drivers/soundwire/master.c
> @@ -161,7 +161,12 @@ int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
>  	/* add shortcuts to improve code readability/compactness */
>  	md->bus = bus;
>  	bus->dev = &md->dev;
> -	bus->md = md;
> +	/*
> +	 * Make sure the contents of md is stored before storing bus->md.
> +	 * Paired with new slave attached and slave status interrupts
> +	 * on the Soundwire master side.
> +	 */
> +	smp_store_release(&bus->md, md);
>  
>  	pm_runtime_set_autosuspend_delay(&bus->md->dev, SDW_MASTER_SUSPEND_DELAY_MS);
>  	pm_runtime_use_autosuspend(&bus->md->dev);