From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752029AbeEFTVA (ORCPT ); Sun, 6 May 2018 15:21:00 -0400 Received: from mail.kernel.org ([198.145.29.99]:39200 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751825AbeEFTUz (ORCPT ); Sun, 6 May 2018 15:20:55 -0400 From: Frederic Weisbecker To: LKML Cc: Frederic Weisbecker , Jiri Olsa , Namhyung Kim , Peter Zijlstra , Linus Torvalds , Yoshinori Sato , Benjamin Herrenschmidt , Catalin Marinas , Chris Zankel , Paul Mackerras , Thomas Gleixner , Will Deacon , Michael Ellerman , Rich Felker , Ingo Molnar , Mark Rutland , Alexander Shishkin , Andy Lutomirski , Arnaldo Carvalho de Melo , Max Filippov Subject: [PATCH 9/9] perf/breakpoint: Only commit breakpoint to arch upon slot reservation success Date: Sun, 6 May 2018 21:19:55 +0200 Message-Id: <1525634395-23380-10-git-send-email-frederic@kernel.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1525634395-23380-1-git-send-email-frederic@kernel.org> References: <1525634395-23380-1-git-send-email-frederic@kernel.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org When slot reservation fails on breakpoint modification, we restore the breakpoint previous state but only halfway as the architecture structure isn't cleaned up. It should be harmless as the breakpoint has to be deactivated at this point. But it's a terrible misdesign. Now that we have split the attribute check and commit code, we can avoid commiting a breakpoint to the architecture until its slot reservation has been accepted and completed. Reported-by: Linus Torvalds Original-patch-by: Andy Lutomirski Signed-off-by: Frederic Weisbecker Cc: Linus Torvalds Cc: Andy Lutomirski Cc: Yoshinori Sato Cc: Rich Felker Cc: Ingo Molnar Cc: Thomas Gleixner Cc: Will Deacon Cc: Mark Rutland Cc: Max Filippov Cc: Chris Zankel Cc: Catalin Marinas Cc: Benjamin Herrenschmidt Cc: Paul Mackerras Cc: Michael Ellerman Cc: Peter Zijlstra Cc: Arnaldo Carvalho de Melo Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Namhyung Kim --- kernel/events/hw_breakpoint.c | 77 ++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 33 deletions(-) diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c index 6896ceeb..f5fbc76 100644 --- a/kernel/events/hw_breakpoint.c +++ b/kernel/events/hw_breakpoint.c @@ -400,14 +400,14 @@ int dbg_release_bp_slot(struct perf_event *bp) return 0; } -static int validate_hw_breakpoint(struct perf_event *bp) +static int hw_breakpoint_check(struct perf_event *bp, + const struct perf_event_attr *attr) { int err; - err = hw_breakpoint_arch_check(bp, &bp->attr); + err = hw_breakpoint_arch_check(bp, attr); if (err) return err; - hw_breakpoint_arch_commit(bp); if (arch_check_bp_in_kernelspace(bp)) { if (bp->attr.exclude_kernel) @@ -425,19 +425,21 @@ static int validate_hw_breakpoint(struct perf_event *bp) int register_perf_hw_breakpoint(struct perf_event *bp) { - int ret; + int err; - ret = reserve_bp_slot(bp); - if (ret) - return ret; + err = reserve_bp_slot(bp); + if (err) + return err; - ret = validate_hw_breakpoint(bp); - - /* if hw_breakpoint_arch_check() fails then release bp slot */ - if (ret) + err = hw_breakpoint_check(bp, &bp->attr); + if (err) { release_bp_slot(bp); + return err; + } - return ret; + hw_breakpoint_arch_commit(bp); + + return 0; } /** @@ -457,35 +459,44 @@ register_user_hw_breakpoint(struct perf_event_attr *attr, } EXPORT_SYMBOL_GPL(register_user_hw_breakpoint); +static void hw_breakpoint_copy_attr(struct perf_event_attr *to, + struct perf_event_attr *from) +{ + to->bp_addr = from->bp_addr; + to->bp_type = from->bp_type; + to->bp_len = from->bp_len; + to->disabled = from->disabled; +} + int modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *attr, bool check) { - 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; + int err; - bp->attr.bp_addr = attr->bp_addr; - bp->attr.bp_type = attr->bp_type; - bp->attr.bp_len = attr->bp_len; - - if (check && memcmp(&bp->attr, attr, sizeof(*attr))) - return -EINVAL; - - 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; + err = hw_breakpoint_check(bp, attr); + if (err) return err; + + if (check) { + struct perf_event_attr old_attr; + + old_attr = bp->attr; + hw_breakpoint_copy_attr(&old_attr, attr); + + if (memcmp(&old_attr, attr, sizeof(*attr))) + return -EINVAL; } - bp->attr.disabled = attr->disabled; + if (bp->attr.bp_type != attr->bp_type) { + err = modify_bp_slot(bp, bp->attr.bp_type); + if (err) + return err; + } + + hw_breakpoint_copy_attr(&bp->attr, attr); + hw_breakpoint_arch_commit(bp); + return 0; } -- 2.7.4