From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3C403C4361A for ; Sat, 5 Dec 2020 19:00:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0270C2310B for ; Sat, 5 Dec 2020 19:00:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726527AbgLETAX (ORCPT ); Sat, 5 Dec 2020 14:00:23 -0500 Received: from out01.mta.xmission.com ([166.70.13.231]:49668 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725267AbgLETAV (ORCPT ); Sat, 5 Dec 2020 14:00:21 -0500 Received: from in01.mta.xmission.com ([166.70.13.51]) by out01.mta.xmission.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1klbwd-00Eu6P-VZ; Sat, 05 Dec 2020 11:06:08 -0700 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1klbwd-0008UB-49; Sat, 05 Dec 2020 11:06:07 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: Davidlohr Bueso Cc: Linus Torvalds , Bernd Edlinger , Linux Kernel Mailing List , Peter Zijlstra , Ingo Molnar , Will Deacon , Jann Horn , Vasiliy Kulikov , Al Viro , Oleg Nesterov , Cyrill Gorcunov , Sargun Dhillon , Christian Brauner , Arnd Bergmann , Arnaldo Carvalho de Melo , Waiman Long References: <87tut2bqik.fsf@x220.int.ebiederm.org> <87ft4mbqen.fsf@x220.int.ebiederm.org> <875z5h4b7a.fsf@x220.int.ebiederm.org> <20201204214836.3rncqw5kox42b4i2@linux-p48b.lan> Date: Sat, 05 Dec 2020 12:05:32 -0600 In-Reply-To: <20201204214836.3rncqw5kox42b4i2@linux-p48b.lan> (Davidlohr Bueso's message of "Fri, 4 Dec 2020 13:48:36 -0800") Message-ID: <878sacyvpv.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1klbwd-0008UB-49;;;mid=<878sacyvpv.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/w+l6vurjxcEGLPmREBqp3weX4WFHRCJY= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw_semaphore X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Davidlohr Bueso writes: > On Fri, 04 Dec 2020, Linus Torvalds wrote: > >>On Fri, Dec 4, 2020 at 12:30 PM Bernd Edlinger >> wrote: >>>> >>> > perf_event_open (exec_update_mutex -> ovl_i_mutex) >> >>Side note: this one looks like it should be easy to fix. >> >>Is there any real reason why exec_update_mutex is actually gotten that >>early, and held for that long in the perf event code? > > afaict just to validate the whole operation early. Per 79c9ce57eb2 the > mutex will guard the check and the perf_install_in_context vs exec. > >> >>I _think_ we could move the ptrace check to be much later, to _just_ before that >> >> * This is the point on no return; we cannot fail hereafter. >> >>point in the perf event install chain.. > > Peter had the idea of doing the ptrace_may_access() check twice: first > lockless and early, then under exec_update_mutex when it mattered right > before perf_install_in_context(): > > https://lore.kernel.org/linux-fsdevel/20200828123720.GZ1362448@hirez.programming.kicks-ass.net/ > >> >>I don't think it needs to be moved down even that much, I think it >>would be sufficient to move it down below the "perf_event_alloc()", >>but I didn't check very much. > > Yeah we could just keep a single ptrace_may_access() check just further > down until it won't deadlock. I am trying to understand why the permission check is there. The ptrace_may_access check came in with the earliest version of the code I can find. So going down that path hasn't helped. There are about 65 calls of perf_pmu_register so it definitely makes me nervous holding a semaphore over perf_event_alloc. What is truly silly in all of this is perf_uprobe_event_init fails if !perfmon_capable(). Which means the ptrace_may_access check and holding exec_update_mutex is completely irrelevant to the function of create_local_trace_uprobe. Which is strange in and of itself. I would have thought uprobes would have been the kind of probe that it would be safe for ordinary users to use. This is a much deeper rabit hole than I had anticipated when I started looking and I will have to come back to this after the weekend. If at all possible I would love to be able to move the grabbing of exec_update_mutex after perf_event_alloc and the pluggable functions it calls. It doesn't feel possible to audit that. On the flip side the task is passed in as hw.target. So it may not be possible to move the permission check. It is chaos. Eric