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=-5.4 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 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 9BFA9C388F9 for ; Fri, 23 Oct 2020 10:49:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3983D21D43 for ; Fri, 23 Oct 2020 10:49:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S462492AbgJWKtg (ORCPT ); Fri, 23 Oct 2020 06:49:36 -0400 Received: from foss.arm.com ([217.140.110.172]:49174 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S462356AbgJWKtg (ORCPT ); Fri, 23 Oct 2020 06:49:36 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 810BE113E; Fri, 23 Oct 2020 03:49:35 -0700 (PDT) Received: from [10.57.13.45] (unknown [10.57.13.45]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 141CA3F66B; Fri, 23 Oct 2020 03:49:30 -0700 (PDT) Subject: Re: [PATCHv2 2/4] coresight: tmc-etf: Fix NULL ptr dereference in tmc_enable_etf_sink_perf() To: Peter Zijlstra Cc: Mathieu Poirier , Sai Prakash Ranjan , Mike Leach , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , coresight@lists.linaro.org, Stephen Boyd , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <20201022113214.GD2611@hirez.programming.kicks-ass.net> <20201022150609.GI2611@hirez.programming.kicks-ass.net> <788706f2-0670-b7b6-a153-3ec6f16e0f2e@arm.com> <20201022212033.GA646497@xps15> <20201023073905.GM2611@hirez.programming.kicks-ass.net> <174e6461-4d46-cb65-c094-c06ee3b21568@arm.com> <20201023092337.GQ2611@hirez.programming.kicks-ass.net> From: Suzuki Poulose Message-ID: <060def4c-ced9-74c5-8e73-d8aedfdbc107@arm.com> Date: Fri, 23 Oct 2020 11:49:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.2 MIME-Version: 1.0 In-Reply-To: <20201023092337.GQ2611@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 10/23/20 10:23 AM, Peter Zijlstra wrote: > On Fri, Oct 23, 2020 at 09:49:53AM +0100, Suzuki Poulose wrote: >> On 10/23/20 8:39 AM, Peter Zijlstra wrote: > >>> So then I don't understand the !->owner issue, that only happens when >>> the task dies, which cannot be concurrent with event creation. Are you >> >> Part of the patch from Sai, fixes this by avoiding the dereferencing >> after event creation (by caching it). But the kernel events needs >> fixing. >> >> One follow up question on the !->owner issue. Given the ->owner is >> dying, does it prevent events from being scheduled ? Or is there a delay >> between that and eventually stopping the events. In this case, we hit >> the issue when : >> >> A A or B ? >> >> event_start() >> ... event->owner = NULL >> >> READ_ONCE(event->owner); >> >> Is this expected ? > > Yeah, teardown is a bit of an effort. Also, you can pass an fd over a > unix socket to another process, so this isn't something you can rely on > in any case. > > The perf tool doesn't do it, but the kernel infra should be able to deal > with someone doing a perf-deamon of sorts, where you can request a perf > event and recieve a fd from it. > > Imagine the fun ;-) > >>> As for the kernel events.. why do you care about the actual task_struct >>> * in there? I see you're using it to grab the task-pid, but how is that >>> useful? >> >> Correct, kernel events are something that the driver didn't account for. >> May be we could handle this case with a "special pid" and simply >> disallow sharing (which is fine I believe, given there are not grouping >> for the kernel created events). > > Why do you need a pid in the first place? Can't you use the "task_struct > *" as a value? We could. But, without a refcount on the task pointer, that could be tricky, even though we don't dereference it. In the same situation, if the tsk owner dies and is freed and is reallocated to a new perf session task but with different PID, we could be mixing things up again ? Special pid here could be -1. 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=-5.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 47D6FC388F9 for ; Fri, 23 Oct 2020 10:51:03 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C22F920FC3 for ; Fri, 23 Oct 2020 10:51:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="UepOj20l" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C22F920FC3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=jUiWY8AUhR/pDetR/e6jamIF6J9UX0AtH6K/DazylZ0=; b=UepOj20lMTSatdJ+N2zVRJ7YE g6WizJM9cvULbuOzGr9kAfmQpH5HCbs4kHkRpx9VTVFjjV3OddSUy8hVjPPXWE9cRqWxZeQ9ibzH3 4Z3xCnHMgGdQ0rnNkN/qtEOm3ihT4x0qD732FidwH1P0zEetMd7KyUsJpgxdleV9nSL/B3QN8+oUe acJPOEFoQwlYOZooyWwpROI9gFZi1QBOPW6FvwZJPALV9BIPHk3vIKDy8XQb2zl91O8RvjMjq/Qo9 kc0RTK1jyjwf9S1g4fC1G1yks3cpZpiMo/Fw6hhlm3+Qfvir3Xv/vM/L3LENX923e3tU1kQTFwHXR zZu/EnfGA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kVudg-0004nu-CK; Fri, 23 Oct 2020 10:49:40 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kVude-0004mW-2J for linux-arm-kernel@lists.infradead.org; Fri, 23 Oct 2020 10:49:39 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 810BE113E; Fri, 23 Oct 2020 03:49:35 -0700 (PDT) Received: from [10.57.13.45] (unknown [10.57.13.45]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 141CA3F66B; Fri, 23 Oct 2020 03:49:30 -0700 (PDT) Subject: Re: [PATCHv2 2/4] coresight: tmc-etf: Fix NULL ptr dereference in tmc_enable_etf_sink_perf() To: Peter Zijlstra References: <20201022113214.GD2611@hirez.programming.kicks-ass.net> <20201022150609.GI2611@hirez.programming.kicks-ass.net> <788706f2-0670-b7b6-a153-3ec6f16e0f2e@arm.com> <20201022212033.GA646497@xps15> <20201023073905.GM2611@hirez.programming.kicks-ass.net> <174e6461-4d46-cb65-c094-c06ee3b21568@arm.com> <20201023092337.GQ2611@hirez.programming.kicks-ass.net> From: Suzuki Poulose Message-ID: <060def4c-ced9-74c5-8e73-d8aedfdbc107@arm.com> Date: Fri, 23 Oct 2020 11:49:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.2 MIME-Version: 1.0 In-Reply-To: <20201023092337.GQ2611@hirez.programming.kicks-ass.net> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201023_064938_303615_A815CAB9 X-CRM114-Status: GOOD ( 25.92 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Sai Prakash Ranjan , Mathieu Poirier , Alexander Shishkin , linux-arm-msm@vger.kernel.org, coresight@lists.linaro.org, linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo , Stephen Boyd , Ingo Molnar , Namhyung Kim , Jiri Olsa , linux-arm-kernel@lists.infradead.org, Mike Leach Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 10/23/20 10:23 AM, Peter Zijlstra wrote: > On Fri, Oct 23, 2020 at 09:49:53AM +0100, Suzuki Poulose wrote: >> On 10/23/20 8:39 AM, Peter Zijlstra wrote: > >>> So then I don't understand the !->owner issue, that only happens when >>> the task dies, which cannot be concurrent with event creation. Are you >> >> Part of the patch from Sai, fixes this by avoiding the dereferencing >> after event creation (by caching it). But the kernel events needs >> fixing. >> >> One follow up question on the !->owner issue. Given the ->owner is >> dying, does it prevent events from being scheduled ? Or is there a delay >> between that and eventually stopping the events. In this case, we hit >> the issue when : >> >> A A or B ? >> >> event_start() >> ... event->owner = NULL >> >> READ_ONCE(event->owner); >> >> Is this expected ? > > Yeah, teardown is a bit of an effort. Also, you can pass an fd over a > unix socket to another process, so this isn't something you can rely on > in any case. > > The perf tool doesn't do it, but the kernel infra should be able to deal > with someone doing a perf-deamon of sorts, where you can request a perf > event and recieve a fd from it. > > Imagine the fun ;-) > >>> As for the kernel events.. why do you care about the actual task_struct >>> * in there? I see you're using it to grab the task-pid, but how is that >>> useful? >> >> Correct, kernel events are something that the driver didn't account for. >> May be we could handle this case with a "special pid" and simply >> disallow sharing (which is fine I believe, given there are not grouping >> for the kernel created events). > > Why do you need a pid in the first place? Can't you use the "task_struct > *" as a value? We could. But, without a refcount on the task pointer, that could be tricky, even though we don't dereference it. In the same situation, if the tsk owner dies and is freed and is reallocated to a new perf session task but with different PID, we could be mixing things up again ? Special pid here could be -1. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel