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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 67BD1C77B72 for ; Thu, 20 Apr 2023 11:25:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234252AbjDTLZd (ORCPT ); Thu, 20 Apr 2023 07:25:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53556 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234232AbjDTLZL (ORCPT ); Thu, 20 Apr 2023 07:25:11 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2DDFD7EE2; Thu, 20 Apr 2023 04:24:06 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6A9F4615C8; Thu, 20 Apr 2023 11:23:48 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1EEEC433D2; Thu, 20 Apr 2023 11:23:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681989827; bh=jXACc+L1gNDsC8WHEpEt3sv4ju+THNheaCP19iu7+kc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=VDfaP9sMJBv4XZlDvCF+L3NIL8S0vLhEDyxGW5wPDHc4W4FtG39RQiMN+ueGJm/aX Z8vtacFBJgR/bIjS/WNirtwYl8Pt/bz03XnLbEBnoepIxn9mg0uIVXSWSj0cdwlqtx S1Dw4FCIgN94ybquFSoEs4pnncjfvmZO2eHnK6ojBpjNbOmmA+jCqQq4enF/GjoqDZ P1I4jsW+SZced8lVKZ7RiXBbCZ2pU2RzWyEDc0jjORk2UQ9eq8y+i4YEjNL58urSFs AQaoa3UbwIvijNgRGgOWRyl8YGBbpLLPLE9eeeaX+8hPjfrYC3Pi61s1YfrO2AOsE/ OoIDI/efGgEbw== Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id D6D99403B5; Thu, 20 Apr 2023 08:23:44 -0300 (-03) Date: Thu, 20 Apr 2023 08:23:44 -0300 From: Arnaldo Carvalho de Melo To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Adrian Hunter , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1] perf test: Fix maps use after put Message-ID: References: <20230420030430.489243-1-irogers@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230420030430.489243-1-irogers@google.com> X-Url: http://acmel.wordpress.com Precedence: bulk List-ID: X-Mailing-List: linux-perf-users@vger.kernel.org Em Wed, Apr 19, 2023 at 08:04:30PM -0700, Ian Rogers escreveu: > Fix a use after put reference count issue. maps is copied from leader, > but the leader is put on line 79 and then maps is used to read the > reference count below - so a use after put, with the put of maps > happening within thread__put. Fix by reversing the order of puts so > that the leader is put last. > > To explain the reference count checker, I wrote this up as a little > example here: > https://perf.wiki.kernel.org/index.php/Reference_Count_Checking > > Note, the bug was introduced by the committer and wasn't present in > the original reference count patch set. Yes, the bug predated your patch and is detected by the reference count checking you contributed. This was just part of splitting up your series into smaller chunks, in this case either we fix the problem detected while developing this reference counting infrastructure or fix it later after the merged infrastructure, when built with EXTRA_CFLAGS="-DREFCNT_CHECKING=1" detects it when running 'perf test'. Thanks for providing the separate patch fixing it. Applied. - Arnaldo > Signed-off-by: Ian Rogers > --- > tools/perf/tests/thread-maps-share.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c > index 75ce8aedfc78..858e725318a9 100644 > --- a/tools/perf/tests/thread-maps-share.c > +++ b/tools/perf/tests/thread-maps-share.c > @@ -76,16 +76,16 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s > TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(other_maps) == RC_CHK_ACCESS(other_leader->maps)); > > /* release thread group */ > - thread__put(leader); > + thread__put(t3); > TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 3); > > - thread__put(t1); > + thread__put(t2); > TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 2); > > - thread__put(t2); > + thread__put(t1); > TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 1); > > - thread__put(t3); > + thread__put(leader); > > /* release other group */ > thread__put(other_leader); > -- > 2.40.0.634.g4ca3ef3211-goog > -- - Arnaldo