From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932303AbdKGRJP (ORCPT <rfc822;w@1wt.eu>);
        Tue, 7 Nov 2017 12:09:15 -0500
Received: from mga09.intel.com ([134.134.136.24]:7678 "EHLO mga09.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1753162AbdKGRJO (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 7 Nov 2017 12:09:14 -0500
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.44,359,1505804400"; 
   d="scan'208";a="4587132"
Date: Tue, 7 Nov 2017 09:09:13 -0800
From: Andi Kleen <ak@linux.intel.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Milind Chabbi <chabbi.milind@gmail.com>, jolsa@redhat.com,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Hari Bathini <hbathini@linux.vnet.ibm.com>,
        Jin Yao <yao.jin@linux.intel.com>, Kan Liang <kan.liang@intel.com>,
        Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf/core: fast breakpoint modification via
 _IOC_MODIFY_BREAKPOINT.
Message-ID: <20171107170913.GF5320@tassilo.jf.intel.com>
References: <1510006156-18988-1-git-send-email-chabbi.milind@gmail.com>
 <20171106231658.GE5320@tassilo.jf.intel.com>
 <20171107081541.GF3326@worktop>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171107081541.GF3326@worktop>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Nov 07, 2017 at 09:15:41AM +0100, Peter Zijlstra wrote:
> On Mon, Nov 06, 2017 at 03:16:58PM -0800, Andi Kleen wrote:
> > > +static int _perf_event_modify_breakpoint(struct perf_event *bp,
> > > +					 struct perf_event_attr *attr)
> > > +{
> > > +	u64 old_addr = bp->attr.bp_addr;
> > > +	u64 old_len = bp->attr.bp_len;
> > > +	int old_type = bp->attr.bp_type;
> > > +	int err = 0;
> > > +
> > > +	_perf_event_disable(bp);
> > > +
> > > +	bp->attr.bp_addr = attr->bp_addr;
> > > +	bp->attr.bp_type = attr->bp_type;
> > > +	bp->attr.bp_len = attr->bp_len;
> > 
> > You don't check any of the other fields, so user space is free
> > to fill in junk. That means they can never be used for anything.
> > It would be better to check at least some of them for being
> > zero, and also that the type matches the break point.
> 
> Yes, the values should at the very least get the exact same validation
> they would get on creating an event with those values.

In this case the ioctl could be also generalized. Not call it _BREAKPOINT,
just _MODIFY. Just for now it would be only implemented for break points,
but that could be potentially extended later.

-Andi