From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f195.google.com (mail-pf1-f195.google.com [209.85.210.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 43E3D2590 for ; Wed, 2 Mar 2022 09:31:31 +0000 (UTC) Received: by mail-pf1-f195.google.com with SMTP id z16so1415885pfh.3 for ; Wed, 02 Mar 2022 01:31:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=FSwFYVuD0cAwR74FpWh/UVnJlZEuhHuzmEJF2CmWuTU5VQk08y1iQ6Cm+4Kk/IHZ7f ykn4dSn+LtVJ/QNPcAOF0Kr9OP0uyQbpGUjZhyE2BsGEuIYzlTgMZwHl/BYpjMpkMG0+ P9mnuSE0YONb1xqLwPVeO/elgzKWhvv7I7OGEBThyQYghI00EIr5RS9MGp7GsCYrqLUi wR7dcG4fdae3Ke7WyU9tBlNyUPSMcC8RmnmBsmKYAmxRcsIUhLqhpYW4uCJTlnf3AogC YxEVbABh4/fwCz1Lhf2xi5Cnii6w5cwNjqEkAadiEwyP0C/cLXPnnAaLLll6ns+CdAHq Ceuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=SsPhF1RmeWCk07kwBEF8VAVqpuEMgHsmw1K8hw3gllHH4xLWzJInx4CziLeyElnV/s QcBzhvKF3sSHaSX7Zo+f6nnSbs4Q1YW6yex+vZRs/p4WrM63uOdVdt279VOKX9foByRh CaLdG/cV7UXcBPJxtkK0GdFHmBp7AAH+LzO0lWhS+tSVANmZxX45smks6B3lhOArul+Y XKe80t30hQMpc+OFNEYyepxTXlSHCDcEoDmTbrBiArlEy52raMHX0HZ/yDysVQaOICjN ydCvdFIWVGa5X9oAeWSHD5M7KCJe8Dqmv0zzcLlgrM3Ch9U38pFEh2VLS7LX8kyGnZHs VXcw== X-Gm-Message-State: AOAM53024LTRIC6feC8hIZPpa8vwYFkw+c7ukU+/Fh7jhtZBxinEGpWX ZFevXFkJUNxO//FergO+TYc= X-Google-Smtp-Source: ABdhPJxxYDNcbSMxl56+YduSTiv9ULKN3/PKEO9PEtlxvCfSyuhc7esxotc8paaSBF6ReGk5V/MJxQ== X-Received: by 2002:a05:6a00:244b:b0:4c9:319e:ecb7 with SMTP id d11-20020a056a00244b00b004c9319eecb7mr31990006pfj.58.1646213490674; Wed, 02 Mar 2022 01:31:30 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.20]) by smtp.googlemail.com with ESMTPSA id y74-20020a62644d000000b004f129e94f40sm19496506pfb.131.2022.03.02.01.31.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 01:31:30 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Cc: akpm@linux-foundation.org, alsa-devel@alsa-project.org, amd-gfx@lists.freedesktop.org, andriy.shevchenko@linux.intel.com, arnd@arndb.de, bcm-kernel-feedback-list@broadcom.com, bjohannesmeyer@gmail.com, c.giuffrida@vu.nl, christian.koenig@amd.com, christophe.jaillet@wanadoo.fr, dan.carpenter@oracle.com, dmaengine@vger.kernel.org, drbd-dev@lists.linbit.com, dri-devel@lists.freedesktop.org, gustavo@embeddedor.com, h.j.bos@vu.nl, intel-gfx@lists.freedesktop.org, intel-wired-lan@lists.osuosl.org, jakobkoschel@gmail.com, jgg@ziepe.ca, keescook@chromium.org, kgdb-bugreport@lists.sourceforge.net, kvm@vger.kernel.org, linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-block@vger.kernel.org, linux-cifs@vger.kernel.org, linux-crypto@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-fsdevel@vger.kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-pm@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-sgx@vger.kernel.org, linux-staging@lists.linux.dev, linux-tegra@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux1394-devel@lists.sourceforge.net, linux@rasmusvillemoes.dk, linuxppc-dev@lists.ozlabs.org, nathan@kernel.org, netdev@vger.kernel.org, nouveau@lists.freedesktop.org, rppt@kernel.org, samba-technical@lists.samba.org, tglx@linutronix.de, tipc-discussion@lists.sourceforge.net, v9fs-developer@lists.sourceforge.net Subject: Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr Date: Wed, 2 Mar 2022 17:31:06 +0800 Message-Id: <20220302093106.8402-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong 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 lists.sourceforge.net (lists.sourceforge.net [216.105.38.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 123EBC433FE for ; Wed, 2 Mar 2022 09:31:41 +0000 (UTC) Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.94.2) (envelope-from ) id 1nPLKe-0006ED-T9; Wed, 02 Mar 2022 09:31:39 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nPLKc-0006Dn-Ur; Wed, 02 Mar 2022 09:31:37 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=References:In-Reply-To:Message-Id:Date:Subject:Cc: To:From:Sender:Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=DN3DlF3jp4ukp1WCoCXgmuewHF 0tanw4geMgRfJ1UF9+ToJKBotRQjflNhzOuilrmPr2PgEn4sUjo1XcuC82J8PtsdDNfhu4AIabjz1 Gt/OLDzjQ9mChjkCj473NMMl9ApPwGNtYB+SjdkWkOSPNgTeL7DZ5UQDBGGxE1h0WzJg=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To :MIME-Version:Content-Type:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=DdjhZOaZAV9XkRCLTeU9rdN/fT qUF+aPlRs41YudJNBLZGgaLcfgdhlGFseNGWE6e0S0GNzunBoUaulzVs5EVlPM3ywjsVTbindGJFN MiWNoqMKn8JUfh+H9LvXM2/xornRhs7cyUZ8mXll5fUezgO5o4bgw+oikYnu3vKCMv8I=; Received: from mail-pf1-f196.google.com ([209.85.210.196]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.94.2) id 1nPLKa-00059Q-9Z; Wed, 02 Mar 2022 09:31:37 +0000 Received: by mail-pf1-f196.google.com with SMTP id d187so1389809pfa.10; Wed, 02 Mar 2022 01:31:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=FSwFYVuD0cAwR74FpWh/UVnJlZEuhHuzmEJF2CmWuTU5VQk08y1iQ6Cm+4Kk/IHZ7f ykn4dSn+LtVJ/QNPcAOF0Kr9OP0uyQbpGUjZhyE2BsGEuIYzlTgMZwHl/BYpjMpkMG0+ P9mnuSE0YONb1xqLwPVeO/elgzKWhvv7I7OGEBThyQYghI00EIr5RS9MGp7GsCYrqLUi wR7dcG4fdae3Ke7WyU9tBlNyUPSMcC8RmnmBsmKYAmxRcsIUhLqhpYW4uCJTlnf3AogC YxEVbABh4/fwCz1Lhf2xi5Cnii6w5cwNjqEkAadiEwyP0C/cLXPnnAaLLll6ns+CdAHq Ceuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=Obkj+VwYef2RgKg8LGIWiWFqaKKv1riyfJaOCpnW5vMfxy2ubvxZVaxz0MmrD5v8nh Z16AD3xPkpdfjsy750uMLkaG4vImkV/vF+mZcK1zQUJgoK6P7EeQV1sGKQDqa45zB02G LsS3HDzk8PR2YUwOaLchc7UcwolMJz9cGmIUO3IAIGGK8+hDNaKfwGLZtNL+pjWrHlek gWIucExxlWw/DbeZdU+iK9KKq3jNKUNB2yc3SKrVqxSA1H1gdfiU0maao4gHEpWy1iVp S4Ym02HbkbclTC79h096NUFsmCLqFMAokb6WyW1JMSmaz3G+rYbL6PWi6eS6AJfmboCu 8opw== X-Gm-Message-State: AOAM532Wy1pRBiH6r+VtBPU17TJXS8sMLtvo5LDow0tGF+jBeJFJNhnI GPu7V92FOhGC1/ZftIuoRl0= X-Google-Smtp-Source: ABdhPJxxYDNcbSMxl56+YduSTiv9ULKN3/PKEO9PEtlxvCfSyuhc7esxotc8paaSBF6ReGk5V/MJxQ== X-Received: by 2002:a05:6a00:244b:b0:4c9:319e:ecb7 with SMTP id d11-20020a056a00244b00b004c9319eecb7mr31990006pfj.58.1646213490674; Wed, 02 Mar 2022 01:31:30 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.20]) by smtp.googlemail.com with ESMTPSA id y74-20020a62644d000000b004f129e94f40sm19496506pfb.131.2022.03.02.01.31.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 01:31:30 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Date: Wed, 2 Mar 2022 17:31:06 +0800 Message-Id: <20220302093106.8402-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: X-Headers-End: 1nPLKa-00059Q-9Z Subject: Re: [f2fs-dev] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr X-BeenThere: linux-f2fs-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, kvm@vger.kernel.org, gustavo@embeddedor.com, linux-iio@vger.kernel.org, kgdb-bugreport@lists.sourceforge.net, linux@rasmusvillemoes.dk, dri-devel@lists.freedesktop.org, c.giuffrida@vu.nl, amd-gfx@lists.freedesktop.org, samba-technical@lists.samba.org, linux1394-devel@lists.sourceforge.net, drbd-dev@lists.linbit.com, linux-arch@vger.kernel.org, linux-cifs@vger.kernel.org, linux-aspeed@lists.ozlabs.org, linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org, linux-staging@lists.linux.dev, h.j.bos@vu.nl, jgg@ziepe.ca, intel-wired-lan@lists.osuosl.org, nouveau@lists.freedesktop.org, bcm-kernel-feedback-list@broadcom.com, dan.carpenter@oracle.com, linux-media@vger.kernel.org, keescook@chromium.org, arnd@arndb.de, linux-pm@vger.kernel.org, intel-gfx@lists.freedesktop.org, bjohannesmeyer@gmail.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, christophe.jaillet@wanadoo.fr, jakobkoschel@gmail.com, v9fs-developer@lists.sourceforge.net, linux-tegra@vger.kernel.org, tglx@linutronix.de, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org, linux-sgx@vger.kernel.org, nathan@kernel.org, netdev@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, tipc-discussion@lists.sourceforge.net, linux-crypto@vger.kernel.org, dmaengine@vger.kernel.org, linux-mediatek@lists.infradead.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org, christian.koenig@amd.com, rppt@kernel.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 758D2C433EF for ; Wed, 2 Mar 2022 09:31:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:MIME-Version:List-Subscribe:List-Help: List-Post:List-Archive:List-Unsubscribe:List-Id:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=IgbNugry4Ew6VOGMDpyZPf5GYCGR2WcjMEhGD+VDZCM=; b=WQ98yO+lOeX+dc irkb2sxaxa1kT6Qkyt6EbborO5vUBErJDKXgEM47BH16csMX7WVUGjn48wsRS/wUfhgyT2TJ4a4aw j2DQ/eYgVhqGAl+l2K9oP5NLMkTSTVWZd5zS5UURE1apNHwksSt0/JIWSVc0dNbl+GQ4bEQscRuS4 IY4n3++vB8wssE3pJPfJR3OXAM/lUD74CDMWf6nVYN8rfa4FTxeE6xkQI4r4af69jUMDyTbtCoMVi /MgH/BhDwb3xVh0MRkmbZavW1lFFEOq3l2EkYQ4vFeckhq7Nc2dUFbcW1Afb7SjJEnirkiYs/Tt/L LjuyPPhUKyUrn76vpDfw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nPLKb-0023qT-Ro; Wed, 02 Mar 2022 09:31:37 +0000 Received: from mail-pf1-x442.google.com ([2607:f8b0:4864:20::442]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nPLKY-0023ot-EA; Wed, 02 Mar 2022 09:31:36 +0000 Received: by mail-pf1-x442.google.com with SMTP id t5so1413329pfg.4; Wed, 02 Mar 2022 01:31:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=FSwFYVuD0cAwR74FpWh/UVnJlZEuhHuzmEJF2CmWuTU5VQk08y1iQ6Cm+4Kk/IHZ7f ykn4dSn+LtVJ/QNPcAOF0Kr9OP0uyQbpGUjZhyE2BsGEuIYzlTgMZwHl/BYpjMpkMG0+ P9mnuSE0YONb1xqLwPVeO/elgzKWhvv7I7OGEBThyQYghI00EIr5RS9MGp7GsCYrqLUi wR7dcG4fdae3Ke7WyU9tBlNyUPSMcC8RmnmBsmKYAmxRcsIUhLqhpYW4uCJTlnf3AogC YxEVbABh4/fwCz1Lhf2xi5Cnii6w5cwNjqEkAadiEwyP0C/cLXPnnAaLLll6ns+CdAHq Ceuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=iqWNK80+oBdMsdgJm4zWQfaP207d6LvVlgBIAtYLMvRng+trc8EYudNhgxi5RAxRmk 9U+a0GlBSEuxYKrmRUQcDCdJw9BltrtPuTqz/LQo/ZM5ZkE9J5HO1P26VIgXVP9BOZbd acucuQSOdNlgr7vSdJLEO6wagzKUnWooiqrwyI4dVzDm6w7BLVDv/qlejcIkA60g7Y0r 9IjNYdhHCFQAOP+gWxBmorJOJ1TTxKWMHI7iGsq3IbSLFu+3LLNXGHFF1/um4H5Um1Y2 qQ6Pozzdv5B82nLuYpXr6z18onn5i8/DM0QcZWXHLEbTctLwQSEJ8oIYuuUfJXkPauqm vYvw== X-Gm-Message-State: AOAM5338GmO7RaDlXj0pzD8Ww8DjzqcJVoiuXrMcRS/yTzaHuw5a7t+1 xdABm9B8njvKUpetbflke+g= X-Google-Smtp-Source: ABdhPJxxYDNcbSMxl56+YduSTiv9ULKN3/PKEO9PEtlxvCfSyuhc7esxotc8paaSBF6ReGk5V/MJxQ== X-Received: by 2002:a05:6a00:244b:b0:4c9:319e:ecb7 with SMTP id d11-20020a056a00244b00b004c9319eecb7mr31990006pfj.58.1646213490674; Wed, 02 Mar 2022 01:31:30 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.20]) by smtp.googlemail.com with ESMTPSA id y74-20020a62644d000000b004f129e94f40sm19496506pfb.131.2022.03.02.01.31.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 01:31:30 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Cc: akpm@linux-foundation.org, alsa-devel@alsa-project.org, amd-gfx@lists.freedesktop.org, andriy.shevchenko@linux.intel.com, arnd@arndb.de, bcm-kernel-feedback-list@broadcom.com, bjohannesmeyer@gmail.com, c.giuffrida@vu.nl, christian.koenig@amd.com, christophe.jaillet@wanadoo.fr, dan.carpenter@oracle.com, dmaengine@vger.kernel.org, drbd-dev@lists.linbit.com, dri-devel@lists.freedesktop.org, gustavo@embeddedor.com, h.j.bos@vu.nl, intel-gfx@lists.freedesktop.org, intel-wired-lan@lists.osuosl.org, jakobkoschel@gmail.com, jgg@ziepe.ca, keescook@chromium.org, kgdb-bugreport@lists.sourceforge.net, kvm@vger.kernel.org, linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-block@vger.kernel.org, linux-cifs@vger.kernel.org, linux-crypto@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-fsdevel@vger.kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-pm@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-sgx@vger.kernel.org, linux-staging@lists.linux.dev, linux-tegra@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux1394-devel@lists.sourceforge.net, linux@rasmusvillemoes.dk, linuxppc-dev@lists.ozlabs.org, nathan@kernel.org, netdev@vger.kernel.org, nouveau@lists.freedesktop.org, rppt@kernel.org, samba-technical@lists.samba.org, tglx@linutronix.de, tipc-discussion@lists.sourceforge.net, v9fs-developer@lists.sourceforge.net Subject: Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr Date: Wed, 2 Mar 2022 17:31:06 +0800 Message-Id: <20220302093106.8402-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220302_013134_487622_E5713837 X-CRM114-Status: GOOD ( 29.97 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek 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 alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F1A67C433F5 for ; Thu, 3 Mar 2022 07:05:10 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 0CB7F1931; Thu, 3 Mar 2022 08:04:19 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 0CB7F1931 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1646291109; bh=QlTEWcqs9PmTj0Hy2M2yZwl5uiwHaskcjZtW7YQqjSs=; h=From:To:Subject:Date:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=DeME8E81QPw8SBWBejW1zq/1M204NFCKIjd0sk5V/C0bYLW2JYTOi5IiHw9HUDfAY 9j/axEBtlgg/YWBfHD8IWq3i/ZNUvuxznmF4Gd+arLgusNyQ6InFBe2Wy2SUtl47rg N615VBaHNOuzu4ay0chrctdmUpltx1jbBQ79Zm6c= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 1C5E9F80515; Thu, 3 Mar 2022 08:03:29 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 72055F801D5; Wed, 2 Mar 2022 10:31:39 +0100 (CET) Received: from mail-pf1-x443.google.com (mail-pf1-x443.google.com [IPv6:2607:f8b0:4864:20::443]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 3023BF80167 for ; Wed, 2 Mar 2022 10:31:32 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 3023BF80167 Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="FSwFYVuD" Received: by mail-pf1-x443.google.com with SMTP id k1so1418916pfu.2 for ; Wed, 02 Mar 2022 01:31:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=FSwFYVuD0cAwR74FpWh/UVnJlZEuhHuzmEJF2CmWuTU5VQk08y1iQ6Cm+4Kk/IHZ7f ykn4dSn+LtVJ/QNPcAOF0Kr9OP0uyQbpGUjZhyE2BsGEuIYzlTgMZwHl/BYpjMpkMG0+ P9mnuSE0YONb1xqLwPVeO/elgzKWhvv7I7OGEBThyQYghI00EIr5RS9MGp7GsCYrqLUi wR7dcG4fdae3Ke7WyU9tBlNyUPSMcC8RmnmBsmKYAmxRcsIUhLqhpYW4uCJTlnf3AogC YxEVbABh4/fwCz1Lhf2xi5Cnii6w5cwNjqEkAadiEwyP0C/cLXPnnAaLLll6ns+CdAHq Ceuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=s/1OvtFFKpDuja4NzBR0hC+Pv74Cl3nmZLiThJEdb7QdUb66ljIkl3zVDABnYYeFsq lB+a1cT7XHYfMEnMqlmVufHWEqwLu3ahRoigmNNRmTt96arxk0TxcEPbAbeHtXcKrrhc IG9AD9CMXHIGWWu0BmQm7BD2csZl2FqRjp38qfeLj58Z3SXSgLe0AV2GT4DjhuAk3Jft 79wn/lsKAuYlNxMu3LBdPvQQBcJeP9S1Qxnj7w6fOrWwrndimNIb3fMHlN+WuKw57AUH q7GhngQztsMZqwqSh13RtuMs/7uPJusj4i3ho90JOqLrY1WOArEDjFkTxzO/qcNIuZsC OOtg== X-Gm-Message-State: AOAM533RuMIRwskBf1slR5FExHhf3Kc3UNe7bYheZlQpZ8atYxvt0Amu ltCJ4SE3A802xgggvDrpd4o= X-Google-Smtp-Source: ABdhPJxxYDNcbSMxl56+YduSTiv9ULKN3/PKEO9PEtlxvCfSyuhc7esxotc8paaSBF6ReGk5V/MJxQ== X-Received: by 2002:a05:6a00:244b:b0:4c9:319e:ecb7 with SMTP id d11-20020a056a00244b00b004c9319eecb7mr31990006pfj.58.1646213490674; Wed, 02 Mar 2022 01:31:30 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.20]) by smtp.googlemail.com with ESMTPSA id y74-20020a62644d000000b004f129e94f40sm19496506pfb.131.2022.03.02.01.31.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 01:31:30 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Subject: Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr Date: Wed, 2 Mar 2022 17:31:06 +0800 Message-Id: <20220302093106.8402-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: X-Mailman-Approved-At: Thu, 03 Mar 2022 08:03:25 +0100 Cc: alsa-devel@alsa-project.org, kvm@vger.kernel.org, gustavo@embeddedor.com, linux-iio@vger.kernel.org, kgdb-bugreport@lists.sourceforge.net, linux@rasmusvillemoes.dk, dri-devel@lists.freedesktop.org, c.giuffrida@vu.nl, amd-gfx@lists.freedesktop.org, samba-technical@lists.samba.org, linux1394-devel@lists.sourceforge.net, drbd-dev@lists.linbit.com, linux-arch@vger.kernel.org, linux-cifs@vger.kernel.org, linux-aspeed@lists.ozlabs.org, linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org, linux-staging@lists.linux.dev, h.j.bos@vu.nl, jgg@ziepe.ca, intel-wired-lan@lists.osuosl.org, nouveau@lists.freedesktop.org, bcm-kernel-feedback-list@broadcom.com, dan.carpenter@oracle.com, linux-media@vger.kernel.org, keescook@chromium.org, arnd@arndb.de, linux-pm@vger.kernel.org, intel-gfx@lists.freedesktop.org, bjohannesmeyer@gmail.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, christophe.jaillet@wanadoo.fr, jakobkoschel@gmail.com, v9fs-developer@lists.sourceforge.net, linux-tegra@vger.kernel.org, tglx@linutronix.de, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org, linux-sgx@vger.kernel.org, nathan@kernel.org, netdev@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, tipc-discussion@lists.sourceforge.net, linux-crypto@vger.kernel.org, dmaengine@vger.kernel.org, linux-mediatek@lists.infradead.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org, christian.koenig@amd.com, rppt@kernel.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong 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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 052B2C433FE for ; Wed, 2 Mar 2022 14:24:31 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 57D0310E1C9; Wed, 2 Mar 2022 14:24:30 +0000 (UTC) Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) by gabe.freedesktop.org (Postfix) with ESMTPS id 283E110F30A; Wed, 2 Mar 2022 09:31:31 +0000 (UTC) Received: by mail-pf1-x444.google.com with SMTP id p8so1397304pfh.8; Wed, 02 Mar 2022 01:31:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=FSwFYVuD0cAwR74FpWh/UVnJlZEuhHuzmEJF2CmWuTU5VQk08y1iQ6Cm+4Kk/IHZ7f ykn4dSn+LtVJ/QNPcAOF0Kr9OP0uyQbpGUjZhyE2BsGEuIYzlTgMZwHl/BYpjMpkMG0+ P9mnuSE0YONb1xqLwPVeO/elgzKWhvv7I7OGEBThyQYghI00EIr5RS9MGp7GsCYrqLUi wR7dcG4fdae3Ke7WyU9tBlNyUPSMcC8RmnmBsmKYAmxRcsIUhLqhpYW4uCJTlnf3AogC YxEVbABh4/fwCz1Lhf2xi5Cnii6w5cwNjqEkAadiEwyP0C/cLXPnnAaLLll6ns+CdAHq Ceuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=AVn/+dEhaFp5DeCDbTgrvMHGGvighq1Z1GogIu1oG/RCpCHIMy1XeChOlDHTBicerw UDj7iJbyvWjgcQnqsdcPERpQIeYgmNJ+vH5R/pMmBcNiAmJ3aV6zXWHXDXMU6VySksEv JiSUGweBL2vwxbLaOnFA14dpAbkty5venJdMjhhYYPR6jtL8/BX2b2Ry97Na0rBnoGM3 jtcbxjEgi9jAMi3aFm7jZz1c+M+Y2Z/1UnrJiXBfkrRb5jwkgX5xwGp8VmSak8FEHHiG XCrRaGocxCsS6Fm9D17SKZcNJ+NbkzkYWmX7QVTklnBxjWAWqE1Wq8YYcc3txekUBXY9 di+A== X-Gm-Message-State: AOAM533K8NAd3vxBl7hDUJeypX6fcVP+yNUxYdvtdbZXdLoH5e6XOUx2 YRJotdM1A0Voqj5/Ew876TU= X-Google-Smtp-Source: ABdhPJxxYDNcbSMxl56+YduSTiv9ULKN3/PKEO9PEtlxvCfSyuhc7esxotc8paaSBF6ReGk5V/MJxQ== X-Received: by 2002:a05:6a00:244b:b0:4c9:319e:ecb7 with SMTP id d11-20020a056a00244b00b004c9319eecb7mr31990006pfj.58.1646213490674; Wed, 02 Mar 2022 01:31:30 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.20]) by smtp.googlemail.com with ESMTPSA id y74-20020a62644d000000b004f129e94f40sm19496506pfb.131.2022.03.02.01.31.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 01:31:30 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Date: Wed, 2 Mar 2022 17:31:06 +0800 Message-Id: <20220302093106.8402-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: X-Mailman-Approved-At: Wed, 02 Mar 2022 14:24:27 +0000 Subject: Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, kvm@vger.kernel.org, gustavo@embeddedor.com, linux-iio@vger.kernel.org, kgdb-bugreport@lists.sourceforge.net, linux@rasmusvillemoes.dk, dri-devel@lists.freedesktop.org, c.giuffrida@vu.nl, amd-gfx@lists.freedesktop.org, samba-technical@lists.samba.org, linux1394-devel@lists.sourceforge.net, drbd-dev@lists.linbit.com, linux-arch@vger.kernel.org, linux-cifs@vger.kernel.org, linux-aspeed@lists.ozlabs.org, linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org, linux-staging@lists.linux.dev, h.j.bos@vu.nl, jgg@ziepe.ca, intel-wired-lan@lists.osuosl.org, nouveau@lists.freedesktop.org, bcm-kernel-feedback-list@broadcom.com, dan.carpenter@oracle.com, linux-media@vger.kernel.org, keescook@chromium.org, arnd@arndb.de, linux-pm@vger.kernel.org, intel-gfx@lists.freedesktop.org, bjohannesmeyer@gmail.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, christophe.jaillet@wanadoo.fr, jakobkoschel@gmail.com, v9fs-developer@lists.sourceforge.net, linux-tegra@vger.kernel.org, tglx@linutronix.de, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org, linux-sgx@vger.kernel.org, nathan@kernel.org, netdev@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, tipc-discussion@lists.sourceforge.net, linux-crypto@vger.kernel.org, dmaengine@vger.kernel.org, linux-mediatek@lists.infradead.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org, christian.koenig@amd.com, rppt@kernel.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong 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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4E396C433EF for ; Tue, 8 Mar 2022 04:27:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E6CED10E65A; Tue, 8 Mar 2022 04:27:28 +0000 (UTC) Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) by gabe.freedesktop.org (Postfix) with ESMTPS id 283E110F30A; Wed, 2 Mar 2022 09:31:31 +0000 (UTC) Received: by mail-pf1-x444.google.com with SMTP id p8so1397304pfh.8; Wed, 02 Mar 2022 01:31:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=FSwFYVuD0cAwR74FpWh/UVnJlZEuhHuzmEJF2CmWuTU5VQk08y1iQ6Cm+4Kk/IHZ7f ykn4dSn+LtVJ/QNPcAOF0Kr9OP0uyQbpGUjZhyE2BsGEuIYzlTgMZwHl/BYpjMpkMG0+ P9mnuSE0YONb1xqLwPVeO/elgzKWhvv7I7OGEBThyQYghI00EIr5RS9MGp7GsCYrqLUi wR7dcG4fdae3Ke7WyU9tBlNyUPSMcC8RmnmBsmKYAmxRcsIUhLqhpYW4uCJTlnf3AogC YxEVbABh4/fwCz1Lhf2xi5Cnii6w5cwNjqEkAadiEwyP0C/cLXPnnAaLLll6ns+CdAHq Ceuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yRnvPGVK9XQdKYPg/H0ecgdZJeSxpiKw/X1pv37GC+w=; b=AVn/+dEhaFp5DeCDbTgrvMHGGvighq1Z1GogIu1oG/RCpCHIMy1XeChOlDHTBicerw UDj7iJbyvWjgcQnqsdcPERpQIeYgmNJ+vH5R/pMmBcNiAmJ3aV6zXWHXDXMU6VySksEv JiSUGweBL2vwxbLaOnFA14dpAbkty5venJdMjhhYYPR6jtL8/BX2b2Ry97Na0rBnoGM3 jtcbxjEgi9jAMi3aFm7jZz1c+M+Y2Z/1UnrJiXBfkrRb5jwkgX5xwGp8VmSak8FEHHiG XCrRaGocxCsS6Fm9D17SKZcNJ+NbkzkYWmX7QVTklnBxjWAWqE1Wq8YYcc3txekUBXY9 di+A== X-Gm-Message-State: AOAM533K8NAd3vxBl7hDUJeypX6fcVP+yNUxYdvtdbZXdLoH5e6XOUx2 YRJotdM1A0Voqj5/Ew876TU= X-Google-Smtp-Source: ABdhPJxxYDNcbSMxl56+YduSTiv9ULKN3/PKEO9PEtlxvCfSyuhc7esxotc8paaSBF6ReGk5V/MJxQ== X-Received: by 2002:a05:6a00:244b:b0:4c9:319e:ecb7 with SMTP id d11-20020a056a00244b00b004c9319eecb7mr31990006pfj.58.1646213490674; Wed, 02 Mar 2022 01:31:30 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.20]) by smtp.googlemail.com with ESMTPSA id y74-20020a62644d000000b004f129e94f40sm19496506pfb.131.2022.03.02.01.31.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Mar 2022 01:31:30 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Date: Wed, 2 Mar 2022 17:31:06 +0800 Message-Id: <20220302093106.8402-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: X-Mailman-Approved-At: Tue, 08 Mar 2022 04:27:27 +0000 Subject: Re: [Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr X-BeenThere: nouveau@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Nouveau development list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, kvm@vger.kernel.org, gustavo@embeddedor.com, linux-iio@vger.kernel.org, kgdb-bugreport@lists.sourceforge.net, linux@rasmusvillemoes.dk, dri-devel@lists.freedesktop.org, c.giuffrida@vu.nl, amd-gfx@lists.freedesktop.org, samba-technical@lists.samba.org, linux1394-devel@lists.sourceforge.net, drbd-dev@lists.linbit.com, linux-arch@vger.kernel.org, linux-cifs@vger.kernel.org, linux-aspeed@lists.ozlabs.org, linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org, linux-staging@lists.linux.dev, h.j.bos@vu.nl, jgg@ziepe.ca, intel-wired-lan@lists.osuosl.org, nouveau@lists.freedesktop.org, bcm-kernel-feedback-list@broadcom.com, dan.carpenter@oracle.com, linux-media@vger.kernel.org, keescook@chromium.org, arnd@arndb.de, linux-pm@vger.kernel.org, intel-gfx@lists.freedesktop.org, bjohannesmeyer@gmail.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, christophe.jaillet@wanadoo.fr, jakobkoschel@gmail.com, v9fs-developer@lists.sourceforge.net, linux-tegra@vger.kernel.org, tglx@linutronix.de, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org, linux-sgx@vger.kernel.org, nathan@kernel.org, netdev@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, tipc-discussion@lists.sourceforge.net, linux-crypto@vger.kernel.org, dmaengine@vger.kernel.org, linux-mediatek@lists.infradead.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org, christian.koenig@amd.com, rppt@kernel.org Errors-To: nouveau-bounces@lists.freedesktop.org Sender: "Nouveau" On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiaomeng Tong Date: Wed, 2 Mar 2022 17:31:06 +0800 Subject: [Intel-wired-lan] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr In-Reply-To: References: Message-ID: <20220302093106.8402-1-xiam0nd.tong@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds wrote: > > But basically to _me_, the important part is that the end result is > maintainable longer-term. I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member) I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong at gmail.com/ Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch. And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers. And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop. Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach. > It turns out that just syntactically, it's really nice to give the > type of the iterator from outside the way we do now. Yeah, it may be a > bit odd, and maybe it's partly because I'm so used to the > "list_for_each_list_entry()" syntax, but moving the type into the loop > construct really made it nasty - either one very complex line, or > having to split it over two lines which was even worse. > > Maybe the place I looked at just happened to have a long typename, but > it's basically always going to be a struct, so it's never a _simple_ > type. And it just looked very odd adn unnatural to have the type as > one of the "arguments" to that list_for_each_entry() macro. we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) { I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts. > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers. > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with. Best regard, -- Xiaomeng Tong