From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763936AbdEWMco (ORCPT ); Tue, 23 May 2017 08:32:44 -0400 Received: from mga04.intel.com ([192.55.52.120]:10161 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1763926AbdEWMcl (ORCPT ); Tue, 23 May 2017 08:32:41 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,382,1491289200"; d="scan'208";a="1151724183" Subject: Re: [PATCH v2] perf/core: Drop kernel samples even though :u is specified To: Mark Rutland Cc: acme@kernel.org, jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org, ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com, will.deacon@arm.com References: <1495534570-3964-1-git-send-email-yao.jin@linux.intel.com> <20170523105820.GC14542@leverpostej> From: "Jin, Yao" Message-ID: <4fd06493-be57-dd80-eb00-ec3fd60270a5@linux.intel.com> Date: Tue, 23 May 2017 20:32:37 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170523105820.GC14542@leverpostej> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org SNIP >> +static bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs) > The name is a bit opaque, especially where it is used in > __perf_event_overflow(). > > How about we invert the polarity and call this sample_is_allowed() ? That's OK, thanks. >> +{ >> + /* >> + * We may get kernel samples even though exclude_kernel >> + * is specified due to potential skid in sampling. >> + * The skid kernel samples could be dropped or just do >> + * nothing by testing the flag PERF_PMU_CAP_NO_SKID. >> + */ >> + if (event->pmu->capabilities & PERF_PMU_CAP_NO_SKID) >> + return false; > Do we need this new cap? > > I'd expect user_mode(regs) to be about as cheap as testing the cap, and > the common case is going to be that we we have test both. > > For those PMUs without skid, when not sampling the kernel, > user_mode(regs) should always be true. > > IMO, it would make more sense to just check user_mode(regs), which also > avoids any surprises with unexpected skid... I guess the reason which Peter recommends to use a new cap is to have a way to keep original behavior. If we don't need to keep original behavior, I think the new cap is not necessary. Could Peter provide comment? Thanks! >> + >> + if (event->attr.exclude_kernel && >> + !user_mode(regs) && >> + (event->attr.sample_type & PERF_SAMPLE_IP)) { >> + return true; >> + } >> + >> + return false; >> +} > How about: > > static bool sample_is_allowed(struct perf_event *event, struct pt_regs *regs) > { > /* > * Due to interrupt latency (AKA "skid"), we may enter the > * kernel before taking an overflow, even if the PMU is only > * counting user events. > * > * To avoid leaking information to userspace, we must always > * reject kernel samples when exclude_kernel is set. > */ > if (!user_mode(regs) && event->attr.exclude_kernel && > (event->attr.sample_type & PERF_SAMPLE_IP)) > return false; > > return true; > } > > ... do we need to reject any other sample types, or do we definitely > avoid leaks by other means? I just think only when the PERF_SAMPLE_IP is applied, we can get correct ip. So I check the PERF_SAMPLE_IP here. >> + >> /* >> * Generic event overflow handling, sampling. >> */ >> @@ -7337,6 +7357,12 @@ static int __perf_event_overflow(struct perf_event *event, >> ret = __perf_event_account_interrupt(event, throttle); >> >> /* >> + * For security, drop the skid kernel samples if necessary. >> + */ >> + if (skid_kernel_samples(event, regs)) >> + return ret; >> + > .. with the above changes, this can be: > > if (!sample_is_allowed(event, regs)) > return ret; > > Thanks, > Mark. OK, thanks! I will change the patch according to your comments. Thanks Jin Yao