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 95F7FC433F5 for ; Fri, 8 Apr 2022 11:42:55 +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:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=W80QYhreAua5CBCxMMmLvtJ8AZAE2B4rHycyirWM9iE=; b=csYg9xAqw+8IlB fjO5wrxMBC2PjE2dKOztLRPonNpx1IPu7vQ/b7l7a6Ad0SSlDFFW81sKHx1hJTxMVphAw6vAyLGQ8 eCW2vUqRjN0HOiRMLmVrVNyNGVg1Ae2YkRDi7kgLpILXW5xdCOjOKlrlIxx0DTLtSgDwvBKaKRWO3 WgT7J7dBHfa6WNXD+39j9T6tZgg4WuPJ6n/lpnw56kFKbSGLiLJsvwtsetgFBhS7ubiwkW/a1edo8 MkXVs1MvMonFpvOuLvd7Ao8HXuMp1Zt6/n2sP8VZ69mUSZ1rzO85ramWTN+rDdYNpQmzu5mW6m+wS BRUZkiGalNgKabi0Nf7Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ncmzd-00GhkJ-GW; Fri, 08 Apr 2022 11:41:34 +0000 Received: from mail-ej1-x633.google.com ([2a00:1450:4864:20::633]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ncmzV-00Ghgs-L8 for linux-arm-kernel@lists.infradead.org; Fri, 08 Apr 2022 11:41:27 +0000 Received: by mail-ej1-x633.google.com with SMTP id ot30so16663237ejb.12 for ; Fri, 08 Apr 2022 04:41:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=oOLkJ7c4AnZzlwaZhpGpMRHcIgXhaokY4dCFfnVK8ik=; b=AJg0uFFwQge4JGuOOuXx8CRkZnu1ZXDnzVzYyDLWHmvIU38NMZoqu0q4pVe158Dkgn /eBGcEWGk1BeH46V8704WX2v2KXvFfp8vmK2QWBZU86JD8zqVMh6WPg5JLEG+MOHz132 joX/LTo9GNjQ/PHxRdkig9FP8sCB1PK8s/CVdG9aD27OA02ni/wYOQv2VKyManpb4r2V l+NyhY4mu+ntHubH7dA6jGBBg6Rw6LlWbypc/PJx+kXrgjcCC5HTa5lEjYMn4vqquVmu OjghRQ+Tl9pBKfNcLM6JX8eao7netCFmQK+me98hABNrns/o3jeMpBRevLlGuLTg9CDP 8pvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=oOLkJ7c4AnZzlwaZhpGpMRHcIgXhaokY4dCFfnVK8ik=; b=UJVxFCm/rbX19v3gnheGKA3l2ZF0mCg1z8E0kfBF6DVyr/kM8b60OVmw7NczG0rlBe G98fNUgI86h6Ftx04IVjW/PPE5AJ6WFphHWvrhuOUZAYLh3rmX58M35TNMLE/GdnylTF b5vskucEFGv8ssRp8wWhAyr9W+hkGYnf4LYGgSpNCUiVHlR/jjEEV+3YXcyODab4n8sX we4dUWhp5OE1gyem4VCI+AOta+MpEKAclslvq9VZgNtAkmUdgUDCNLno1qK0LmAhzWEp csvW1hjQssoQrGrrY1+QEy8nArfsCd1fHr3zwuEzH6DyMvqQ+AIIhBAkiquQcQ2OnDVx ALnw== X-Gm-Message-State: AOAM532J/cyPcvUzi8U6aK+jA36DoNxaaMQXR5aJahMs1cacHex6/vDK hPN9oCSLKEbwlwoWAZSHIls= X-Google-Smtp-Source: ABdhPJxeGaH+7ttKQKVamBraqnau+HZGtwku7UJ842hOx0+3i3iRgWvMoiWyLlCCBLIqlNq60ltPnQ== X-Received: by 2002:a17:907:2d92:b0:6e8:4b2a:e41f with SMTP id gt18-20020a1709072d9200b006e84b2ae41fmr3203383ejc.124.1649418083561; Fri, 08 Apr 2022 04:41:23 -0700 (PDT) Received: from skbuf ([188.26.57.45]) by smtp.gmail.com with ESMTPSA id e11-20020a50becb000000b0041b64129200sm10750300edk.50.2022.04.08.04.41.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 08 Apr 2022 04:41:23 -0700 (PDT) Date: Fri, 8 Apr 2022 14:41:20 +0300 From: Vladimir Oltean To: Jakob Koschel Cc: "David S. Miller" , Jakub Kicinski , Paolo Abeni , Andrew Lunn , Vivien Didelot , Florian Fainelli , Lars Povlsen , Steen Hegelund , UNGLinuxDriver@microchip.com, Ariel Elior , Manish Chopra , Edward Cree , Martin Habets , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Jiri Pirko , Casper Andersson , Bjarni Jonasson , Colin Ian King , Michael Walle , Christophe JAILLET , Arnd Bergmann , Eric Dumazet , Di Zhu , Xu Wang , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, Mike Rapoport , Brian Johannesmeyer , Cristiano Giuffrida , "Bos, H.J." Subject: Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop Message-ID: <20220408114120.tvf2lxvhfqbnrlml@skbuf> References: <20220407102900.3086255-1-jakobkoschel@gmail.com> <20220407102900.3086255-3-jakobkoschel@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220407102900.3086255-3-jakobkoschel@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220408_044125_790636_2EB548DC X-CRM114-Status: GOOD ( 38.70 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hello Jakob, On Thu, Apr 07, 2022 at 12:28:47PM +0200, Jakob Koschel wrote: > In preparation to limit the scope of a list iterator to the list > traversal loop, use a dedicated pointer to point to the found element [1]. > > Before, the code implicitly used the head when no element was found > when using &pos->list. Since the new variable is only set if an > element was found, the list_add() is performed within the loop > and only done after the loop if it is done on the list head directly. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > Signed-off-by: Jakob Koschel > --- > drivers/net/dsa/sja1105/sja1105_vl.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c > index b7e95d60a6e4..cfcae4d19eef 100644 > --- a/drivers/net/dsa/sja1105/sja1105_vl.c > +++ b/drivers/net/dsa/sja1105/sja1105_vl.c > @@ -27,20 +27,24 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, > if (list_empty(&gating_cfg->entries)) { > list_add(&e->list, &gating_cfg->entries); > } else { > - struct sja1105_gate_entry *p; > + struct sja1105_gate_entry *p = NULL, *iter; > > - list_for_each_entry(p, &gating_cfg->entries, list) { > - if (p->interval == e->interval) { > + list_for_each_entry(iter, &gating_cfg->entries, list) { > + if (iter->interval == e->interval) { > NL_SET_ERR_MSG_MOD(extack, > "Gate conflict"); > rc = -EBUSY; > goto err; > } > > - if (e->interval < p->interval) > + if (e->interval < iter->interval) { > + p = iter; > + list_add(&e->list, iter->list.prev); > break; > + } > } > - list_add(&e->list, p->list.prev); > + if (!p) > + list_add(&e->list, gating_cfg->entries.prev); > } > > gating_cfg->num_entries++; > -- > 2.25.1 > I apologize in advance if I've misinterpreted the end goal of your patch. I do have a vague suspicion I understand what you're trying to achieve, and in that case, would you mind using this patch instead of yours? I think it still preserves the intention of the code in a clean manner. -----------------------------[ cut here ]----------------------------- >From 7aed740750d1bc3bff6e85fd33298f5905bb4e01 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Fri, 8 Apr 2022 13:55:14 +0300 Subject: [PATCH] net: dsa: sja1105: avoid use of type-confused pointer in sja1105_insert_gate_entry() It appears that list_for_each_entry() leaks a type-confused pointer when the iteration loop ends with no early break, since "*p" will no longer point to a "struct sja1105_gate_entry", but rather to some memory in front of "gating_cfg->entries". This isn't actually a problem here, because if the element we insert has the highest interval, therefore we never exit the loop early, "p->list" (which is all that we use outside the loop) will in fact point to "gating_cfg->entries" even though "p" itself is invalid. Nonetheless, there are preparations to increase the safety of list_for_each_entry() by making it impossible to use the encapsulating structure of the iterator element outside the loop. So something needs to change here before those preparations go in, even though this constitutes legitimate use. Make it clear that we are not dereferencing members of the encapsulating "struct sja1105_gate_entry" outside the loop, by using the regular list_for_each() iterator, and dereferencing the struct sja1105_gate_entry only within the loop. With list_for_each(), the iterator element at the end of the loop does have a sane value in all cases, and we can just use that as the "head" argument of list_add(). Signed-off-by: Vladimir Oltean --- drivers/net/dsa/sja1105/sja1105_vl.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c index c0e45b393fde..fe93c80fe5ef 100644 --- a/drivers/net/dsa/sja1105/sja1105_vl.c +++ b/drivers/net/dsa/sja1105/sja1105_vl.c @@ -27,9 +27,15 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, if (list_empty(&gating_cfg->entries)) { list_add(&e->list, &gating_cfg->entries); } else { - struct sja1105_gate_entry *p; + struct list_head *pos; + + /* We cannot safely use list_for_each_entry() + * because we dereference "pos" after the loop + */ + list_for_each(pos, &gating_cfg->entries) { + struct sja1105_gate_entry *p; - list_for_each_entry(p, &gating_cfg->entries, list) { + p = list_entry(pos, struct sja1105_gate_entry, list); if (p->interval == e->interval) { NL_SET_ERR_MSG_MOD(extack, "Gate conflict"); @@ -40,7 +46,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, if (e->interval < p->interval) break; } - list_add(&e->list, p->list.prev); + list_add(&e->list, pos->prev); } gating_cfg->num_entries++; -----------------------------[ cut here ]----------------------------- _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel