linux-kernel - Re: [PATCH v3 06/13] coresight: Fix loss of connection info when a module is unloaded

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: <bd5bbc22-4e5e-df1c-fcd1-bef0e55b6db8@arm.com>
Date:   Thu, 30 Mar 2023 15:01:06 +0100
From:   Suzuki K Poulose <suzuki.poulose@....com>
To:     James Clark <james.clark@....com>, coresight@...ts.linaro.org,
        quic_jinlmao@...cinc.com, mike.leach@...aro.org
Cc:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v3 06/13] coresight: Fix loss of connection info when a
 module is unloaded

On 30/03/2023 13:42, Suzuki K Poulose wrote:
> Hi James
> 
> On 29/03/2023 12:53, James Clark wrote:
>> child_fwnode should be a read only property based on the DT. If it's
>> cleared on the parent device when a child is unloaded, then when the
>> child is loaded again the connection won't be remade.
>>
>> child_dev should be cleared instead which signifies that the connection
>> should be remade when the child_fwnode registers a new coresight_device.
>>
>> Similarly the reference count shouldn't be decremented as long as the
>> parent device exists. The correct place to drop the reference is in
>> coresight_release_platform_data() which is already done.
>>
>> Signed-off-by: James Clark <james.clark@....com>
> 
> This looks like a bug in the existing driver. Please could you keep this
> at the beginning of the series, so that it is easier to backport for
> stable ?
> 
> Also, please add :
> 
> Fixes: 37ea1ffddffa ("coresight: Use fwnode handle instead of device 
> names")
> Fixes: 2af89ebacf29 ("coresight: Clear the connection field properly")
> 
> 
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index bfe1b93aca4e..12cbb68e8e1c 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1414,13 +1414,8 @@ static int coresight_remove_match(struct device 
>> *dev, void *data)
>>           if (csdev->dev.fwnode == conn->dest_fwnode) {
>>               iterator->orphan = true;
>>               coresight_remove_links(iterator, conn);
>> -            /*
>> -             * Drop the reference to the handle for the remote
>> -             * device acquired in parsing the connections from
>> -             * platform data.
>> -             */
>> -            fwnode_handle_put(conn->dest_fwnode);
>> -            conn->dest_fwnode = NULL;
>> +
>> +            conn->dest_dev = NULL;
>>               /* No need to continue */
>>               break;
>>           }
>> @@ -1553,7 +1548,6 @@ void coresight_release_platform_data(struct 
>> coresight_device *csdev,
>>            * is going away
>>            */
>>           fwnode_handle_put(conns[i].dest_fwnode);
>> -        pdata->out_conns[i].dest_fwnode = NULL;
> 
> This change is not required, as we are freeing the pdata anyway.
> Keeping this might be beneficial if someone else comes looking
> for references while we are going out. I don't see how we could
> hit that, but just to be safe.

For reference, I could reproduce the problem by :

On juno, load all coresight modules.

$ cd /sys/bus/coresight/devices/
$ echo 1 > tmc_etr0/enable_sink
$ echo 1 > etm0/enable_source

Works fine ^

$ echo 0 > etm0/enable_source
$ rmmod coresight-funnel
$ modprobe coresight-funnel
$ echo 1 > etm0/enable_source
-bash: echo: write error: Invalid argument

Suzuki

> 
> Suzuki
>