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.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_2 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 1E7A7C64E7A for ; Tue, 1 Dec 2020 21:29:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 57348208FE for ; Tue, 1 Dec 2020 21:29:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 57348208FE Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id BF2676B0068; Tue, 1 Dec 2020 16:29:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B7C888D0002; Tue, 1 Dec 2020 16:29:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A1DB78D0001; Tue, 1 Dec 2020 16:29:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0064.hostedemail.com [216.40.44.64]) by kanga.kvack.org (Postfix) with ESMTP id 8742D6B0068 for ; Tue, 1 Dec 2020 16:29:01 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 40F91824999B for ; Tue, 1 Dec 2020 21:29:01 +0000 (UTC) X-FDA: 77546003682.13.limit27_4b0739b273ad Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin13.hostedemail.com (Postfix) with ESMTP id 1833F18140B8A for ; Tue, 1 Dec 2020 21:28:53 +0000 (UTC) X-HE-Tag: limit27_4b0739b273ad X-Filterd-Recvd-Size: 4276 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf40.hostedemail.com (Postfix) with ESMTP for ; Tue, 1 Dec 2020 21:28:52 +0000 (UTC) Received: from gandalf.local.home (cpe-66-24-58-225.stny.res.rr.com [66.24.58.225]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 477C920870; Tue, 1 Dec 2020 21:28:49 +0000 (UTC) Date: Tue, 1 Dec 2020 16:28:47 -0500 From: Steven Rostedt To: Axel Rasmussen Cc: Andrew Morton , Chinwen Chang , Daniel Jordan , David Rientjes , Davidlohr Bueso , Ingo Molnar , Jann Horn , Laurent Dufour , Michel Lespinasse , Stephen Rothwell , Vlastimil Babka , Yafang Shao , davem@davemloft.net, dsahern@kernel.org, gregkh@linuxfoundation.org, kuba@kernel.org, liuhangbin@gmail.com, tj@kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v2] mm: mmap_lock: fix use-after-free race and css ref leak in tracepoints Message-ID: <20201201162847.654f3013@gandalf.local.home> In-Reply-To: <20201201203249.4172751-1-axelrasmussen@google.com> References: <20201201203249.4172751-1-axelrasmussen@google.com> X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, 1 Dec 2020 12:32:49 -0800 Axel Rasmussen wrote: > +/* Called with reg_lock held. */ The above comment is reduntant, as the lockdep_is_held() below also suggest that it is ;-) > +static void free_memcg_path_bufs(void) > +{ > + int cpu; > + char *old; > + > + for_each_possible_cpu(cpu) { > + old = rcu_dereference_protected(per_cpu(memcg_path_buf, cpu), > + lockdep_is_held(®_lock)); > + if (old == NULL) > + break; Hmm, what if the topology of the system has missing CPU numbers (this is possible I believe on some systems)? > + rcu_assign_pointer(per_cpu(memcg_path_buf, cpu), NULL); > + /* Wait for inflight memcg_path_buf users to finish. */ > + synchronize_rcu(); Please break this up into two loops. You will need to have another array that is created in trace_mmap_lock_reg() function: static char **path_holders; trace_mmap_lock_reg() { [..] path_holders = kmalloc(num_possible_cpus * sizeof(*path_holders)); [..] } Then this function can be: static void free_memcg_path_bufs(void) { int cpu; for_each_possible_cpu(cpu) { path_holders[cpu] = rcu_dereference_protected(per_cpu(memcg_path_buf, cpu), lockdep_is_held(®_lock)); rcu_assign_pointer(per_cpu(memcg_path_buf, cpu), NULL); } /* Wait for inflight memcg_path_buf users to finish. */ synchronize_rcu(); for_each_possible_cpu(cpu) { kfree(path_holders[cpu]); } kfree(path_holders); path_holders = NULL; } Otherwise, if you have a machine with 128 possible CPUs, doing 128 synchronize_rcu()s is going to be expensive! > + kfree(old); > + } > +} > > static inline char *get_memcg_path_buf(void) > { > + char *buf; > int idx; > > + rcu_read_lock(); The caller of get_mm_memcg_path() has preemption disabled, which is also now an RCU lock. So the rcu_read_lock() is somewhat redundant. Oh, and looking at the original patch: + memcg_path != NULL ? memcg_path : "", \ The above could be shorten to: memcg_path ? : "", As gcc has a trick with the "? :" which is if there's nothing in between the "?" and ":" it will use what was tested as the result if it is not zero or NULL. -- Steve > + buf = rcu_dereference(*this_cpu_ptr(&memcg_path_buf)); > + if (buf == NULL) > + return NULL; > idx = this_cpu_add_return(memcg_path_buf_idx, MEMCG_PATH_BUF_SIZE) - > MEMCG_PATH_BUF_SIZE; > - return &this_cpu_read(memcg_path_buf)[idx]; > + return &buf[idx]; > }