linux-kernel - Re: [PATCH v1 7/9]powerpc/powernv: Event attr creation and PMU registration

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: <1433293592.438.74.camel@axtens.net>
Date:	Wed, 03 Jun 2015 11:06:32 +1000
From:	Daniel Axtens <dja@...ens.net>
To:	Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	Stephane Eranian <eranian@...gle.com>,
	Paul Mackerras <paulus@...ba.org>,
	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
Subject: Re: [PATCH v1 7/9]powerpc/powernv: Event attr creation and PMU
 registration

On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote:
> Patch adds common event attribute function and Nest pmu registration call.
> 
> Cc: Michael Ellerman <mpe@...erman.id.au>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Cc: Paul Mackerras <paulus@...ba.org>
> Cc: Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
> Cc: Anshuman Khandual <khandual@...ux.vnet.ibm.com>
> Cc: Stephane Eranian <eranian@...gle.com>
> Signed-off-by: Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>
> ---
>  arch/powerpc/perf/nest-pmu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c
> index 514a0be..dd84fd7 100644
> --- a/arch/powerpc/perf/nest-pmu.c
> +++ b/arch/powerpc/perf/nest-pmu.c
> @@ -244,6 +244,49 @@ static int update_pmu_ops(struct nest_pmu *pmu)
>  	return 0;
>  }
>  
> +/*
> + * Populate event name and string in attribute
> + */
> +struct attribute *dev_str_attr(char *name, char *str)
> +{
> +	struct perf_pmu_events_attr *attr;
> +
> +	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> +
> +	attr->event_str = (const char *)str;
Erk. Two things:
 - Is str const or not? If you're treating it as const here, should you
pass that through the function signature?
 - Who is responsible for the memory behind it? It looks like a caller
can't construct str dynamically, pass it to this function and then free
it, because that will invalidate attr->event_str. Is this documented?

> +	attr->attr.attr.name = name;
> +	attr->attr.attr.mode = 0444;
> +	attr->attr.show = perf_event_sysfs_show;
> +
> +	return &attr->attr.attr;
If you're returning the address of attr->attr.attr, then:
 - why don't you just deal directly with struct attribute * in the
function? Why an entire struct perf_pmu_events_attr *?
 - with the function as written, if you return just &attr->attr.attr,
don't attr->event_str and attr->attr.show get lost?

> +}
> +
> +int update_events_in_group(
> +	struct ppc64_nest_ima_events *p8_events, int idx,
> +		struct nest_pmu *pmu)
> +{
> +	struct attribute_group *attr_group;
> +	struct attribute **attrs;
> +	int i;
> +
> +	attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) +
> +				sizeof(*attr_group)), GFP_KERNEL);
> +	if (!attr_group)
> +		return -ENOMEM;
> +
> +	attrs = (struct attribute **)(attr_group + 1);
> +	attr_group->name = "events";
> +	attr_group->attrs = attrs;
> +
> +	for (i=0; i< idx; i++, p8_events++)
> +		attrs[i] = dev_str_attr((char *)p8_events->ev_name,
> +						(char *)p8_events->ev_value);
> +
> +	pmu->attr_groups[0] = attr_group;
> +	return 0;
> +}
I'm very confused by what this function is trying to do. Could you add
some comments? I'm particularly confused by the relationship between
attrs and attr_group.

> +
> +
>  static int nest_pmu_create(struct device_node *dev, int pmu_index)
>  {
>  	struct ppc64_nest_ima_events **p8_events_arr;
> @@ -364,6 +407,15 @@ static int nest_pmu_create(struct device_node *dev, int pmu_index)
>  		}
>  	}
>  
> +	update_events_in_group(
> +		(struct ppc64_nest_ima_events *)p8_events_arr,
> +							idx, pmu_ptr);
> +	update_pmu_ops(pmu_ptr);
> +
> +	/* Register the pmu */
> +	perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
> +	printk(KERN_INFO "Nest PMU %s Registered\n", pmu_ptr->pmu.name);
> +
>  	return 0;
>  }
>  

Regards,
Daniel


Download attachment "signature.asc" of type "application/pgp-signature" (861 bytes)