Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   David Laight <David.Laight@ACULAB.COM>
To:     "'kan.liang@linux.intel.com'" <kan.liang@linux.intel.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "ak@linux.intel.com" <ak@linux.intel.com>
Subject: RE: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address
 before accessing
Thread-Topic: [PATCH V2 3/3] perf/x86/intel/uncore: Validate MMIO address
 before accessing
Thread-Index: AQHWNPJS+Ezh9RvMC0GYZtjdLUC73Ki9fXeg
Date:   Thu, 28 May 2020 13:33:00 +0000
Message-ID: <de3b847eddd143998997d70a1ba161b8@AcuMS.aculab.com>
References: <1590671727-99311-1-git-send-email-kan.liang@linux.intel.com>
 <1590671727-99311-3-git-send-email-kan.liang@linux.intel.com>
In-Reply-To: <1590671727-99311-3-git-send-email-kan.liang@linux.intel.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: kan.liang@linux.intel.com
> Sent: 28 May 2020 14:15
...
> +static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
> +					unsigned long offset)

You need a better name, needs to start 'uncore_' and 'mmio'
probably isn't right either.

> +{
> +	if (box->pmu->type->mmio_map_size > offset)
> +		return true;

Swap over.
Conditionals always read best if 'variable op constant'.
So you want:
	if (offset < box->pmu->type->mmio_map_size)
		return true;

> +
> +	pr_warn_once("perf uncore: Access invalid address of %s.\n",
> +		     box->pmu->type->name);

Pretty hard to debug without the invalid offset.

I've no idea what the code is supposed to be doing though.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)