Received: by 2002:a6b:fb09:0:0:0:0:0 with SMTP id h9csp665893iog; Mon, 13 Jun 2022 10:10:34 -0700 (PDT) X-Google-Smtp-Source: AGRyM1vY+YTXq42ASJkU0sAFGgfJ9i2SSwr0S/TXAaxVGvpjgvlsoXyzY//FLnS3AO+jc4JHgcqF X-Received: by 2002:a17:902:b597:b0:168:d8ce:4a63 with SMTP id a23-20020a170902b59700b00168d8ce4a63mr28941pls.57.1655140234263; Mon, 13 Jun 2022 10:10:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1655140234; cv=none; d=google.com; s=arc-20160816; b=DyWCHLNkVxyqaiQP0Hkwrvl9j3+a59sIhXhf6IX2c1b4FyRmwmOlchr4j6fCdJSR62 +z8GCf95NykDV15AEji6wqdwpS2bgZzCODoc7qgOEeP4pDReXDa71jbY1BD8frHadK1r zim3PB0IG3xLSLZETDz0oAABJpag+u+jvvreeTe8nWDpCNg0xKg1Xt6+ECqmF9MYNuyH NfXeVfN5Ox9Viev1Ymc3iqtjbSilsPzCZgxZr6rbxeAgUV8VrCtNuAVy9rQTyf0BGf6j 5pFokIvJ/NFWNOwsR11P5mpXNkCa4TkbvJyjj2WptTVyY5XHAgmyfjEjydsqn7Gbcr9h Seog== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=wypwNo31pEoKbTgQWNWX70zzoyNQ9oMUPSN5tbzhBBg=; b=c077S/8LbCmba9pz14JwMiVao3+g87uUOZwApSzRWCVdtKJbM/tw3c84dBfl+w7I05 Xu82C3e/AxSESdPU9X9sgMQfr2Yu7TjA8HS0FzEKEWKhk8UmvWA1DzhwXJMstJ3gTStd BJxVBez80nc4SPKyeQkasqrFWLwPPIKgHhCEbNcz9Bl8GkFKxgWxUnarxGyywouc2wi6 aY+omQUxm665raijnAKOdMqDAScig1Y2yADf64P/2/rrocX50o9LlgkeVlw/IUqCkSn8 W0Qu4zkqqmYI249+1OviCkK8l7Gvv2iTpqBjbzRAUd5Bynqs48DGa2PLSxUTegi+YHt6 79Uw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@infradead.org header.s=casper.20170209 header.b=QOaOTcl7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id h21-20020a63e155000000b003fc1bb74001si9942803pgk.54.2022.06.13.10.10.21; Mon, 13 Jun 2022 10:10:34 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@infradead.org header.s=casper.20170209 header.b=QOaOTcl7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238939AbiFMQyU (ORCPT + 99 others); Mon, 13 Jun 2022 12:54:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44290 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243147AbiFMQxq (ORCPT ); Mon, 13 Jun 2022 12:53:46 -0400 Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:8b0:10b:1236::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 607491F5899 for ; Mon, 13 Jun 2022 07:38:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=wypwNo31pEoKbTgQWNWX70zzoyNQ9oMUPSN5tbzhBBg=; b=QOaOTcl7RlOJ50kzhpL16tQ+O/ AUGcSiZNJDKL/1WyLtbcDrKtQMC2cyooJMJuZxTrE1Xl/cdIKus7LrijAyhz6AUPw04LvqqM9NWkt oYO7Qz6zBrX7qU+V7BVnfke89MVmym3N+9IKpWN1nKkRAyjINSISNTciDPkgnF5mU+PCm1KwTUu3D f+rj2u+rdOcElXQclEB67mG8hF/YLMZYrCAH76Ca1Ct9lcpatDkLDwuYMz4brQooSGZXNTsuECMjj oXT92E73xUbgX4TeOGQsYzmRqntv6I4nbTq4iIQvIdboy8wIyz4D4vy38mHuVE9Ghouu0s17rP9TR AL6C7Rxg==; Received: from dhcp-077-249-017-003.chello.nl ([77.249.17.3] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1o0lDE-00Gv6S-NB; Mon, 13 Jun 2022 14:38:40 +0000 Received: from hirez.programming.kicks-ass.net (hirez.programming.kicks-ass.net [192.168.1.225]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by noisy.programming.kicks-ass.net (Postfix) with ESMTPS id 558FB30023F; Mon, 13 Jun 2022 16:38:40 +0200 (CEST) Received: by hirez.programming.kicks-ass.net (Postfix, from userid 1000) id 3E12E2029F884; Mon, 13 Jun 2022 16:38:40 +0200 (CEST) Date: Mon, 13 Jun 2022 16:38:40 +0200 From: Peter Zijlstra To: Ravi Bangoria Cc: acme@kernel.org, alexander.shishkin@linux.intel.com, jolsa@redhat.com, namhyung@kernel.org, songliubraving@fb.com, eranian@google.com, alexey.budankov@linux.intel.com, ak@linux.intel.com, mark.rutland@arm.com, megha.dey@intel.com, frederic@kernel.org, maddy@linux.ibm.com, irogers@google.com, kim.phillips@amd.com, linux-kernel@vger.kernel.org, santosh.shukla@amd.com Subject: Re: [RFC v2] perf: Rewrite core context handling Message-ID: References: <20220113134743.1292-1-ravi.bangoria@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 13, 2022 at 04:35:11PM +0200, Peter Zijlstra wrote: Another one of those lockdep splats: > @@ -12147,42 +12256,37 @@ SYSCALL_DEFINE5(perf_event_open, > if (pmu->task_ctx_nr == perf_sw_context) > event->event_caps |= PERF_EV_CAP_SOFTWARE; > > - if (group_leader) { > - if (is_software_event(event) && > - !in_software_context(group_leader)) { > - /* > - * If the event is a sw event, but the group_leader > - * is on hw context. > - * > - * Allow the addition of software events to hw > - * groups, this is safe because software events > - * never fail to schedule. > - */ > - pmu = group_leader->ctx->pmu; > - } else if (!is_software_event(event) && > - is_software_event(group_leader) && > - (group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) { > - /* > - * In case the group is a pure software group, and we > - * try to add a hardware event, move the whole group to > - * the hardware context. > - */ > - move_group = 1; > - } > - } > - > /* > * Get the target context (task or percpu): > */ > - ctx = find_get_context(pmu, task, event); > + ctx = find_get_context(task, event); > if (IS_ERR(ctx)) { > err = PTR_ERR(ctx); > goto err_alloc; > } > > - /* > - * Look up the group leader (we will attach this event to it): > - */ > + mutex_lock(&ctx->mutex); > + > + if (ctx->task == TASK_TOMBSTONE) { > + err = -ESRCH; > + goto err_locked; > + } > + > + if (!task) { > + /* > + * Check if the @cpu we're creating an event for is online. > + * > + * We use the perf_cpu_context::ctx::mutex to serialize against > + * the hotplug notifiers. See perf_event_{init,exit}_cpu(). > + */ > + struct perf_cpu_context *cpuctx = per_cpu_ptr(&cpu_context, event->cpu); > + > + if (!cpuctx->online) { > + err = -ENODEV; > + goto err_locked; > + } > + } > + > if (group_leader) { > err = -EINVAL; > pulling up the ctx->mutex makes things simpler, but also violates the locking order vs exec_update_lock. Pull that lock up as well... --- --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -12254,13 +12254,29 @@ SYSCALL_DEFINE5(perf_event_open, if (pmu->task_ctx_nr == perf_sw_context) event->event_caps |= PERF_EV_CAP_SOFTWARE; + if (task) { + err = down_read_interruptible(&task->signal->exec_update_lock); + if (err) + goto err_alloc; + + /* + * We must hold exec_update_lock across this and any potential + * perf_install_in_context() call for this new event to + * serialize against exec() altering our credentials (and the + * perf_event_exit_task() that could imply). + */ + err = -EACCES; + if (!perf_check_permission(&attr, task)) + goto err_cred; + } + /* * Get the target context (task or percpu): */ ctx = find_get_context(task, event); if (IS_ERR(ctx)) { err = PTR_ERR(ctx); - goto err_alloc; + goto err_cred; } mutex_lock(&ctx->mutex); @@ -12358,58 +12374,14 @@ SYSCALL_DEFINE5(perf_event_open, goto err_context; } - event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, f_flags); - if (IS_ERR(event_file)) { - err = PTR_ERR(event_file); - event_file = NULL; - goto err_context; - } - - if (task) { - err = down_read_interruptible(&task->signal->exec_update_lock); - if (err) - goto err_file; - - /* - * We must hold exec_update_lock across this and any potential - * perf_install_in_context() call for this new event to - * serialize against exec() altering our credentials (and the - * perf_event_exit_task() that could imply). - */ - err = -EACCES; - if (!perf_check_permission(&attr, task)) - goto err_cred; - } - - if (ctx->task == TASK_TOMBSTONE) { - err = -ESRCH; - goto err_locked; - } - if (!perf_event_validate_size(event)) { err = -E2BIG; - goto err_locked; - } - - if (!task) { - /* - * Check if the @cpu we're creating an event for is online. - * - * We use the perf_cpu_context::ctx::mutex to serialize against - * the hotplug notifiers. See perf_event_{init,exit}_cpu(). - */ - struct perf_cpu_context *cpuctx = - container_of(ctx, struct perf_cpu_context, ctx); - - if (!cpuctx->online) { - err = -ENODEV; - goto err_locked; - } + goto err_context; } if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader)) { err = -EINVAL; - goto err_locked; + goto err_context; } /* @@ -12418,11 +12390,18 @@ SYSCALL_DEFINE5(perf_event_open, */ if (!exclusive_event_installable(event, ctx)) { err = -EBUSY; - goto err_cred; + goto err_context; } WARN_ON_ONCE(ctx->parent_ctx); + event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, f_flags); + if (IS_ERR(event_file)) { + err = PTR_ERR(event_file); + event_file = NULL; + goto err_context; + } + /* * This is the point on no return; we cannot fail hereafter. This is * where we start modifying current state. @@ -12500,17 +12479,15 @@ SYSCALL_DEFINE5(perf_event_open, fd_install(event_fd, event_file); return event_fd; -err_cred: - if (task) - up_read(&task->signal->exec_update_lock); -err_file: - fput(event_file); err_context: /* event->pmu_ctx freed by free_event() */ err_locked: mutex_unlock(&ctx->mutex); perf_unpin_context(ctx); put_ctx(ctx); +err_cred: + if (task) + up_read(&task->signal->exec_update_lock); err_alloc: /* * If event_file is set, the fput() above will have called ->release()