From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932209AbdK0RZi (ORCPT ); Mon, 27 Nov 2017 12:25:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58450 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932100AbdK0RZh (ORCPT ); Mon, 27 Nov 2017 12:25:37 -0500 Date: Mon, 27 Nov 2017 18:25:32 +0100 From: Jiri Olsa To: Peter Zijlstra Cc: Jiri Olsa , Ingo Molnar , Arnaldo Carvalho de Melo , lkml , Namhyung Kim , David Ahern , Andi Kleen , Milind Chabbi , Alexander Shishkin , Michael Ellerman , Hari Bathini , Jin Yao , Kan Liang , Sukadev Bhattiprolu , Oleg Nesterov , Will Deacon Subject: Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function Message-ID: <20171127172532.GA23094@krava> References: <20171127162133.21163-1-jolsa@kernel.org> <20171127162133.21163-5-jolsa@kernel.org> <20171127164639.3ymnc6io3eae7n4c@hirez.programming.kicks-ass.net> <20171127170911.GA22026@krava> <20171127171203.tmdvcsnsownieijv@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171127171203.tmdvcsnsownieijv@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.9.1 (2017-09-22) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 27 Nov 2017 17:25:37 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 27, 2017 at 06:12:03PM +0100, Peter Zijlstra wrote: > On Mon, Nov 27, 2017 at 06:09:11PM +0100, Jiri Olsa wrote: > > On Mon, Nov 27, 2017 at 05:46:39PM +0100, Peter Zijlstra wrote: > > > On Mon, Nov 27, 2017 at 05:21:31PM +0100, Jiri Olsa wrote: > > > > +static int __modify_user_hw_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; > > > > + bool modify = attr->bp_type != old_type; > > > > + int err = 0; > > > > + > > > > + bp->attr.bp_addr = attr->bp_addr; > > > > + bp->attr.bp_type = attr->bp_type; > > > > + bp->attr.bp_len = attr->bp_len; > > > > + > > > > + err = validate_hw_breakpoint(bp); > > > > + if (!err && modify) > > > > + err = modify_bp_slot(bp, old_type); > > > > + > > > > + if (err) { > > > > + bp->attr.bp_addr = old_addr; > > > > + bp->attr.bp_type = old_type; > > > > + bp->attr.bp_len = old_len; > > > > + return err; > > > > + } > > > > + > > > > + bp->attr.disabled = attr->disabled; > > > > + return 0; > > > > +} > > > > > > I think this function is failing to check if anything else in the attr > > > changes. > > > > > > For example, someone could have added PERF_SAMPLE_BRANCH_STACK. That's > > > something you'll fail to create breakpoints with, but this modification > > > would 'accept'. > > > > > > > hum, I dont think so.. the only things you're allowed to change > > are bp_addr, bp_type and bp_len.. we put new values in those > > fields and keep the rest untouched.. apart from 'disabled' bit > > But what validates the input attr is the same as the event attr, aside > from those fields? we don't.. the attr serves as a holder to carry those fields into the function the current kernel interface does not check anything else there's one more check in the ioctl path, we check the type in perf_event_modify_attr: if (event->attr.type != attr->type) return -EINVAL; jirka