linux-kernel - RE: [PATCH v5] perf: arm_cspmu: Separate Arm and vendor module

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:   Fri, 11 Aug 2023 00:53:03 +0000
From:   Besar Wicaksono <bwicaksono@...dia.com>
To:     Will Deacon <will@...nel.org>,
        "suzuki.poulose@....com" <suzuki.poulose@....com>
CC:     "robin.murphy@....com" <robin.murphy@....com>,
        "ilkka@...amperecomputing.com" <ilkka@...amperecomputing.com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        Thierry Reding <treding@...dia.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Vikram Sethi <vsethi@...dia.com>,
        Richard Wiley <rwiley@...dia.com>,
        Eric Funsten <efunsten@...dia.com>
Subject: RE: [PATCH v5] perf: arm_cspmu: Separate Arm and vendor module

Hi Will,

Please see my reply inline.

> -----Original Message-----
> From: Will Deacon <will@...nel.org>
> Sent: Friday, July 28, 2023 8:22 AM
> To: Besar Wicaksono <bwicaksono@...dia.com>; suzuki.poulose@....com
> Cc: robin.murphy@....com; ilkka@...amperecomputing.com;
> catalin.marinas@....com; mark.rutland@....com; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> tegra@...r.kernel.org; Thierry Reding <treding@...dia.com>; Jonathan
> Hunter <jonathanh@...dia.com>; Vikram Sethi <vsethi@...dia.com>; Richard
> Wiley <rwiley@...dia.com>; Eric Funsten <efunsten@...dia.com>
> Subject: Re: [PATCH v5] perf: arm_cspmu: Separate Arm and vendor module
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jul 05, 2023 at 05:47:45AM -0500, Besar Wicaksono wrote:
> > Arm Coresight PMU driver consists of main standard code and
> > vendor backend code. Both are currently built as a single module.
> > This patch adds vendor registration API to separate the two to
> > keep things modular. The main driver requests each known backend
> > module during initialization and defer device binding process.
> > The backend module then registers an init callback to the main
> > driver and continue the device driver binding process.
> >
> > Signed-off-by: Besar Wicaksono <bwicaksono@...dia.com>
> > ---
> >
> > Changes from v4:
> >  * Fix warning reported by kernel test robot
> > v4: https://lore.kernel.org/linux-arm-kernel/20230620041438.32514-1-
> bwicaksono@...dia.com/T/#u
> 
> One minor comment below, but this mostly looks good to me. I'd like Suzuki's
> Ack before I queue it, though.
> 
> > +     /* Load implementer module and initialize the callbacks. */
> > +     if (match) {
> > +             mutex_lock(&arm_cspmu_lock);
> > +
> > +             if (match->impl_init_ops) {
> > +                     if (try_module_get(match->module)) {
> > +                             cspmu->impl.match = match;
> > +                             ret = match->impl_init_ops(cspmu);
> > +                             module_put(match->module);
> 
> Why is it safe to drop the module reference here? If I'm understanding the
> flow correctly, ->impl_init_ops() will populate more function pointers
> in the cspmu->impl.ops structure, and we don't appear to take a module
> reference when calling those.
> 
> What happens if the backend module is unloaded while the core module
> is executed those functions?
> 

We also update the call to perf_pmu_register and provide the backend module handle.
The core perf kernel will acquire the reference on the backend module prior to calling the
functions in cspmu->imp.ops. Please see the change below

+static inline struct module *arm_cspmu_get_module(struct arm_cspmu *cspmu)
+{
+	return (cspmu->impl.match) ? cspmu->impl.match->module : THIS_MODULE;
+}
+
 static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 {
 	int ret, capabilities;
@@ -1149,7 +1173,7 @@ static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
 
 	cspmu->pmu = (struct pmu){
 		.task_ctx_nr	= perf_invalid_context,
-		.module		= THIS_MODULE,
+		.module		= arm_cspmu_get_module(cspmu),
 		.pmu_enable	= arm_cspmu_enable,
 		.pmu_disable	= arm_cspmu_disable,
 		.event_init	= arm_cspmu_event_init,

Regards,
Besar