linux-kernel - Re: [PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable

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: <20170118182055.wpgo5b5xe3m4nrql@pd.tnic>
Date:   Wed, 18 Jan 2017 19:20:55 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
Cc:     linux-kernel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        joro@...tes.org, peterz@...radead.org, mingo@...hat.com
Subject: Re: [PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable_event

On Mon, Jan 16, 2017 at 01:23:29AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> 
> * Clean up various bitwise operations in perf_iommu_enable_event
								  ()

It is a function.

> * Make use macros BIT(x)

"Make use"?

> This should not affect logic and functionality.
> 
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Borislav Petkov <bp@...en8.de>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> ---
>  arch/x86/events/amd/iommu.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 44638d0..1aa25d8 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -164,11 +164,11 @@ static int get_next_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu)
>  	for (bank = 0, shift = 0; bank < max_banks; bank++) {
>  		for (cntr = 0; cntr < max_cntrs; cntr++) {
>  			shift = bank + (bank*3) + cntr;
> -			if (perf_iommu->cntr_assign_mask & (1ULL<<shift)) {
> +			if (perf_iommu->cntr_assign_mask & BIT(shift)) {

							   BIT_ULL()

otherwise you're introducing a bug.

>  				continue;
>  			} else {
> -				perf_iommu->cntr_assign_mask |= (1ULL<<shift);
> -				retval = ((u16)((u16)bank<<8) | (u8)(cntr));
> +				perf_iommu->cntr_assign_mask |= BIT(shift);

Ditto.

> +				retval = ((u16)((u16)bank << 8) | (u8)(cntr));

That's some ugly casting. Why?

(u8)(cntr) is supposed to do what exactly? Cut off to 255 if max_cntrs
grows bigger? Can that even happen?

Same for bank?

Then you're casting to u16 even though retval is int. Ah, because it can
be negative too.

How about this instead?

	retval = ((bank & 0xff) << 8) | (cntr & 0xff);

This way it is really obvious what you're trying to do.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.