by Suzuki K Poulose

[permalink] [raw]

Subject: Re: [PATCH v3 13/13] coresight: Fix CTI module refcount leak by making it a helper device

On 04/04/2023 14:04, James Clark wrote:
>
>
> On 04/04/2023 13:55, James Clark wrote:
>>
>>
>> On 04/04/2023 10:21, Suzuki K Poulose wrote:
>>> On 29/03/2023 12:53, James Clark wrote:
>>>> The CTI module has some hard coded refcounting code that has a leak.
>>>> For example running perf and then trying to unload it fails:
>>>>
>>>>    perf record -e cs_etm// -a -- ls
>>>>    rmmod coresight_cti
>>>>
>>>>    rmmod: ERROR: Module coresight_cti is in use
>>>>
>>>> The coresight core already handles references of devices in use, so by
>>>> making CTI a normal helper device, we get working refcounting for free.
>>>>
>>>> Signed-off-by: James Clark <[email protected]>
>>>> ---
>>>> drivers/hwtracing/coresight/coresight-core.c | 99 ++++++-------------
>>>> .../hwtracing/coresight/coresight-cti-core.c | 52 +++++-----
>>>> .../hwtracing/coresight/coresight-cti-sysfs.c | 4 +-
>>>> drivers/hwtracing/coresight/coresight-cti.h   | 4 +-
>>>> drivers/hwtracing/coresight/coresight-priv.h | 4 +-
>>>> drivers/hwtracing/coresight/coresight-sysfs.c | 4 +
>>>> include/linux/coresight.h                     | 30 +-----
>>>> 7 files changed, 70 insertions(+), 127 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>>>> b/drivers/hwtracing/coresight/coresight-core.c
>>>> index 65f5bd8516d8..458d91b4e23f 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>> @@ -254,60 +254,39 @@ void coresight_disclaim_device(struct
>>>> coresight_device *csdev)
>>>> }
>>>> EXPORT_SYMBOL_GPL(coresight_disclaim_device);
>>>> -/* enable or disable an associated CTI device of the supplied CS
>>>> device */
>>>> -static int
>>>> -coresight_control_assoc_ectdev(struct coresight_device *csdev, bool
>>>> enable)
>>>> -{
>>>> -    int ect_ret = 0;
>>>> -    struct coresight_device *ect_csdev = csdev->ect_dev;
>>>> -    struct module *mod;
>>>> -
>>>> -    if (!ect_csdev)
>>>> -        return 0;
>>>> -    if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
>>>> -        return 0;
>>>> -
>>>> -    mod = ect_csdev->dev.parent->driver->owner;
>>>> -    if (enable) {
>>>> -        if (try_module_get(mod)) {
>>>> -            ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
>>>> -            if (ect_ret) {
>>>> -                module_put(mod);
>>>> -            } else {
>>>> -                get_device(ect_csdev->dev.parent);
>>>> -                csdev->ect_enabled = true;
>>>> -            }
>>>> -        } else
>>>> -            ect_ret = -ENODEV;
>>>> -    } else {
>>>> -        if (csdev->ect_enabled) {
>>>> -            ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
>>>> -            put_device(ect_csdev->dev.parent);
>>>> -            module_put(mod);
>>>> -            csdev->ect_enabled = false;
>>>> -        }
>>>> -    }
>>>> -
>>>> -    /* output warning if ECT enable is preventing trace operation */
>>>> -    if (ect_ret)
>>>> -        dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
>>>> -             dev_name(&ect_csdev->dev),
>>>> -             enable ? "enable" : "disable");
>>>> -    return ect_ret;
>>>> -}
>>>> -
>>>> /*
>>>> - * Set the associated ect / cti device while holding the coresight_mutex
>>>> + * Add a helper as an output device while holding the coresight_mutex
>>>>    * to avoid a race with coresight_enable that may try to use this
>>>> value.
>>>>    */
>>>> -void coresight_set_assoc_ectdev_mutex(struct coresight_device *csdev,
>>>> -                      struct coresight_device *ect_csdev)
>>>> +void coresight_add_helper_mutex(struct coresight_device *csdev,
>>>> +                struct coresight_device *helper)
>>>
>>> minor nit: It may be a good idea to rename this, in line with the
>>> kernel naming convention :
>>>
>>>     coresight_add_helper_unlocked()
>>>
>>> Or if this is the only variant, it is OK to leave it as :
>>>     coresight_add_helper()
>>> with a big fat comment in the function description to indicate
>>> that it takes the mutex and may be even add a :
>>>
>> There is already a bit of a comment in the description but I can expand
>> on it more.
>>
>>> might_sleep() and lockdep_assert_not_held(&coresight_mutex);
>>>
>>> in the function.
>>>
>>
>> I'm not sure if lockdep_assert_not_held() would be right because
>> sometimes it could be held if another device is being created at the
>> same time? Or something like a session is started at the same time a CTI
>> device is added.
>>
>
> Oh I see it's not for any task, it's just for the current one. That
> makes sense then I can add it.
>
> Although it looks like it only warns when lockdep is enabled, but don't
> you get a warning anyway if you try to take the lock twice with lockdep
> enabled?

Thats true, you could ignore the lockdep check.

So I'm not sure why we would add lockdep_assert_not_held() here
> and not on all the mutex_lock() calls?\

Ah. I double checked this and the coresight_mutex is static and local to
coresight-core.c. So there is no point in talking about locking for
external users. So I would just leave out any suffixes and simply use
the lockdep check implicit from mutex_lock().

Suzuki