linux-kernel - Re: [2/2] coresight: Fix reference count for software sources

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:   Wed, 15 Mar 2017 16:00:32 +0000
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Chunyan Zhang <zhang.chunyan@...aro.org>
Cc:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [2/2] coresight: Fix reference count for software sources

On 15/03/17 03:51, Chunyan Zhang wrote:
> Hi Suzuki,
>
> On 15 March 2017 at 02:06, Suzuki K Poulose <Suzuki.Poulose@....com> wrote:
>> On 14/03/17 17:40, Mathieu Poirier wrote:
>>>
>>> On 14 March 2017 at 11:32, Mathieu Poirier <mathieu.poirier@...aro.org>
>>> wrote:
>>>>
>>>> From: "Suzuki K. Poulose" <Suzuki.Poulose@....com>
>>>>
>>>> For software sources (i.e STM), there could be multiple agents
>>>> generating the trace data, unlike the ETMs. So we need to
>>>> properly do the accounting for the active number of users
>>>> to disable the device when the last user goes away. Right
>>>> now, the reference counting is broken for sources as we skip
>>>> the actions when we detect that the source is enabled.
>>>>
>>>> This patch fixes the problem by adding the refcounting for
>>>> software sources, even when they are enabled.
>>>>
>>>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>>>> Reported-by: Robert Walker <robert.walker@....com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>>>> ---
>>>>  drivers/hwtracing/coresight/coresight.c | 13 +++++++++++--
>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight.c
>>>> b/drivers/hwtracing/coresight/coresight.c
>>>> index 34cd1ed..2da9e39 100644
>>>> --- a/drivers/hwtracing/coresight/coresight.c
>>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>>> @@ -552,6 +552,7 @@ int coresight_enable(struct coresight_device *csdev)
>>>>         int cpu, ret = 0;
>>>>         struct coresight_device *sink;
>>>>         struct list_head *path;
>>>> +       enum coresight_dev_subtype_source subtype =
>>>> csdev->subtype.source_subtype;
>>>
>>>
>>> Checkpatch.pl complains about a line over 80 characters.
>>>
>>>>
>>>>         mutex_lock(&coresight_mutex);
>>>>
>>>> @@ -559,8 +560,16 @@ int coresight_enable(struct coresight_device *csdev)
>>>>         if (ret)
>>>>                 goto out;
>>>>
>>>> -       if (csdev->enable)
>>>> +       if (csdev->enable) {
>>>> +               /*
>>>> +                * There could be multiple applications driving the
>>>> software
>>>> +                * source. So keep the refcount for each such user when
>>>> the
>>>> +                * source is already enabled.
>>>> +                */
>>>> +               if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE)
>>>> +                       atomic_inc(csdev->refcnt);
>>>>                 goto out;
>>>> +       }
>>>>
>>
>> Btw, should we allow the user to turn on the STM from sysfs (echo 1 >
>> $STM/enable_source) ?
>
> If enabling STM can not be allowed via sysfs, how should we allow
> users to turn on STM when they want to mmap STM to user space, and
> write STM device from user space directly? For example this kind of
> use case [1].

The ioct(, STP_POLICY_ID_SET) indirectly turns on the STM hardware via :
stm_char_policy_set_ioctl()->stm.link (stm_generic_link)-> coresight_enable().


>
>> All STM users should set their policy via ioctls and that in turn turns the
>> device on.
>
> Yes users can set policy via ioctls to request resource of STM (i.e.
> which STM channel(s) will be written), but they still need to use
> sysfs to enable STM.

As mentioned above, it is not necessary.

>
>> So it doesn't make sense for enable_source to really enable
>> the hardware unless someone really opens it.
>
> Right, there're two ways to enable STM currently, e.g.
> 1) echo <addr>.stm  > /sys/class/stm_source/stm_ftrace/stm_source_link

I am not familiar with the stm_source class. From a quick glance, it looks like,
writing to stm_source_link triggers :
  stm_source_link_store()->stm_source_link_add()->(stm->data->link()).

which is fine for connecting a source (ftrace,console or heartbeat) to STM.

> 2) echo 1 > $STM/enable_source

This is a bit awkward for an application who wants to mmap and stream data,
and is quite unnecessary from my explanation above.

>
> That would probably make people confused, I would appreciate any
> better solution.

Please let me know if you have any outstanding concerns.

Cheers
Suzuki