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=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 9619BC43603 for ; Wed, 18 Dec 2019 09:47:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 72B0F218AC for ; Wed, 18 Dec 2019 09:47:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725955AbfLRJrl (ORCPT ); Wed, 18 Dec 2019 04:47:41 -0500 Received: from Galois.linutronix.de ([193.142.43.55]:57182 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725799AbfLRJrl (ORCPT ); Wed, 18 Dec 2019 04:47:41 -0500 Received: from bigeasy by Galois.linutronix.de with local (Exim 4.80) (envelope-from ) id 1ihVvd-0005Rx-IR; Wed, 18 Dec 2019 10:47:37 +0100 Date: Wed, 18 Dec 2019 10:47:37 +0100 From: Sebastian Andrzej Siewior To: Rob Herring Cc: devicetree@vger.kernel.org, Frank Rowand , "linux-kernel@vger.kernel.org" , Michael Ellerman , Segher Boessenkool Subject: Re: [PATCH] of: Rework and simplify phandle cache to use a fixed size Message-ID: <20191218094737.tqq5oeajfgvds6n5@linutronix.de> References: <20191211232345.24810-1-robh@kernel.org> <20191212130539.loxpr2hbfcodh4gz@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 2019-12-12 13:28:26 [-0600], Rob Herring wrote: > On Thu, Dec 12, 2019 at 7:05 AM Sebastian Andrzej Siewior > wrote: > > > > On 2019-12-11 17:48:54 [-0600], Rob Herring wrote: > > > > - if (phandle_cache) { > > > > - if (phandle_cache[masked_handle] && > > > > - handle == phandle_cache[masked_handle]->phandle) > > > > - np = phandle_cache[masked_handle]; > > > > - if (np && of_node_check_flag(np, OF_DETACHED)) { > > > > - WARN_ON(1); /* did not uncache np on node removal */ > > > > - of_node_put(np); > > > > - phandle_cache[masked_handle] = NULL; > > > > - np = NULL; > > > > - } > > > > + if (phandle_cache[handle_hash] && > > > > + handle == phandle_cache[handle_hash]->phandle) > > > > + np = phandle_cache[handle_hash]; > > > > + if (np && of_node_check_flag(np, OF_DETACHED)) { > > > > + WARN_ON(1); /* did not uncache np on node removal */ > > > > > > BTW, I don't think this check is even valid. If we failed to detach > > > and remove the node from the cache, then we could be accessing np > > > after freeing it. > > > > this is kmalloc()ed memory which is always valid. If the memory is > > already re-used then > > handle == phandle_cache[handle_hash]->phandle > > > > will fail (the check, not the memory access itself). > > There's a 1 in 2^32 chance it won't. :) > > If the check > > remains valid then you can hope for the OF_DETACHED flag to trigger the > > warning. > > Keyword is hope. > > To look at it another way. Do we need this check? It is in the "fast > path". There's a single location where we set OF_DETACHED and the > cache entry is removed at the same time. Also, if we do free the > node's memory, it also checks for OF_DETACHED. Previously, a free > wouldn't happen because we incremented the ref count on nodes in the > cache. So get rid of it then. It is just __of_detach_node() that removes it. > Rob Sebastian