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 D3271C433F5 for ; Sun, 10 Apr 2022 18:25:51 +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:To:References:Message-Id:Cc:Date: In-Reply-To:From:Subject:Mime-Version:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=BKz81ezRlnVgk3/j4VfFg9l8bx/vS+ZMIB2fKhPkayE=; b=QTigGjpSOaQoCx 8TvTKB/0f44kj4wxo5dhP14n8m7zC+O1wUpekBCB5WJergRojQ5i6UUgRrHCTlIVSWUpOuj5gkOOl DahznhVchMKLSZ7kPAv95swXcN1TgBt2fWU0dD+JHo/CMRX/Z77IpIBzr50sjDrBLy0OoyHrFIohl Gfglc6/pHdwgDAJCCycsJ/lhcz1f2f8OeioZgJGXinEZrKlwm9QtuzQyIepQnv4ISTfhVEMiN4Ba8 +6FZF4SqKO+NjJXE1GBNlz+wJ6/XT1fytJYUQ0SvHPjij81KZyngXoQ9KeWiRP6TWbYHRBRkn+snE 9vXUdV9h31Xj+qYTdwig==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ndcEv-005Hba-7t; Sun, 10 Apr 2022 18:24:45 +0000 Received: from mail-ed1-x52c.google.com ([2a00:1450:4864:20::52c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ndcEr-005Hav-NT for linux-arm-kernel@lists.infradead.org; Sun, 10 Apr 2022 18:24:43 +0000 Received: by mail-ed1-x52c.google.com with SMTP id b24so15729931edu.10 for ; Sun, 10 Apr 2022 11:24:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=6cdVUS6XiscYrUgxUxtAL2ssNdCpR5fNQ/PXRqx8HXE=; b=E3oamHlVUO9sG+tUlCb7w/lxckY+1bMJ8Oj/JMRsdthREWTAo/EU1ssZbsEddX/CMW XF+LLnOvzqbQ09/kqpDMUFnXICqi/jhgYhhv5gxrC5MMdDeloz3+bT7nNdHFZfSEL2q0 XjhyXlJ2wmJ+nWvvMT0izK3719NdGolLrqWsrZQa3UBq1r9OQ40xF0MyV4NqmK82mhIJ Z5+8pR6KZwFLv6NnK3nKzBo3rK6QovFRSGe9xxfzi7Cygm/LyX76sqMrbY0iZzA1pxUb HTenU1x8AjD/thUEi/iR5ReeV/7u+ZMAdb3HHejyOJt71iosXVU5vKr6vibgesiixUDv Ojhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=6cdVUS6XiscYrUgxUxtAL2ssNdCpR5fNQ/PXRqx8HXE=; b=OIRvotgQZQ0ZBHlwODA8VJOhYD3cJFDYcyz71lukC2OknBqqD540k5+lzSdH8eRFSM CrIyGsKCD+6utB3WSkkqmw5ieGlURXC/ueXO69DWMQl+CR6g07DB4RwgPOhDgaYvvUk2 bww1Q/hzoePWnKP1AMWbF2rREodaoPCaweHIcgDByRGCSZvh/FIKy9y51UPVqCeDur+N 9qt38FOOoWB4ogu74UhsZMMahbQAd/XZImEL+yp6aN/Fk1vHyIdL63N4KR+M+dMDMNo+ xJl1A66aoirnVyggJUfFq4522bqnuXWDiQRkkd/BzNVGNR9YljRBJRPgEkcNiDggfgrs jFQw== X-Gm-Message-State: AOAM532+mSxU702ZhWwF0X+r5g/0Ov7jr9J+cvHg/x1O7oBwjbKKBO4e p7q7PxHC8Wc39RaBnh5o62U= X-Google-Smtp-Source: ABdhPJzHtBkIeS+/EEn5hhsp2OBGrQi0n93cEQMWNioFwMIDx3qPQINg9OZCTpvVlSgC58g1HvsjRQ== X-Received: by 2002:aa7:db94:0:b0:410:f0e8:c39e with SMTP id u20-20020aa7db94000000b00410f0e8c39emr29340972edt.14.1649615079652; Sun, 10 Apr 2022 11:24:39 -0700 (PDT) Received: from smtpclient.apple (i130160.upc-i.chello.nl. [62.195.130.160]) by smtp.gmail.com with ESMTPSA id s4-20020a170906bc4400b006e893908c4csm356693ejv.60.2022.04.10.11.24.38 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 10 Apr 2022 11:24:39 -0700 (PDT) Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\)) Subject: Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop From: Jakob Koschel In-Reply-To: <935062D0-C657-4C79-A0BE-70141D052EC0@gmail.com> Date: Sun, 10 Apr 2022 20:24:37 +0200 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 , LKML , Linux ARM , linuxppc-dev , Mike Rapoport , Brian Johannesmeyer , Cristiano Giuffrida , "Bos, H.J." Message-Id: References: <20220407102900.3086255-1-jakobkoschel@gmail.com> <20220407102900.3086255-3-jakobkoschel@gmail.com> <20220408114120.tvf2lxvhfqbnrlml@skbuf> <20220410110508.em3r7z62ufqcbrfm@skbuf> <935062D0-C657-4C79-A0BE-70141D052EC0@gmail.com> To: Vladimir Oltean X-Mailer: Apple Mail (2.3696.80.82.1.1) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220410_112441_811477_2A5CBEE8 X-CRM114-Status: GOOD ( 35.87 ) 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 > On 10. Apr 2022, at 14:39, Jakob Koschel wrote: > > > >> On 10. Apr 2022, at 13:05, Vladimir Oltean wrote: >> >> On Sun, Apr 10, 2022 at 12:51:56PM +0200, Jakob Koschel wrote: >>> I've just looked at this again in a bit more detail while integrating it into the patch series. >>> >>> I realized that this just shifts the 'problem' to using the 'pos' iterator variable after the loop. >>> If the scope of the list iterator would be lowered to the list traversal loop it would also make sense >>> to also do it for list_for_each(). >> >> Yes, but list_for_each() was never formulated as being problematic in >> the same way as list_for_each_entry(), was it? I guess I'm starting to >> not understand what is the true purpose of the changes. > > Sorry for having caused the confusion. Let me elaborate a bit to give more context. > > There are two main benefits of this entire effort. > > 1) fix the architectural bugs and avoid any missuse of the list iterator after the loop > by construction. This only concerns the list_for_each_entry_*() macros and your change > will allow lowering the scope for all of those. It might be debatable that it would be > more consistent to lower the scope for list_for_each() as well, but it wouldn't be > strictly necessary. > > 2) fix *possible* speculative bugs. In our project, Kasper [1], we were able to show > that this can be an issue for the list traversal macros (and this is how the entire > effort started). > The reason is that the processor might run an additional loop iteration in speculative > execution with the iterator variable computed based on the head element. This can > (and we have verified this) happen if the CPU incorrectly > assumes !list_entry_is_head(pos, head, member). > > If this happens, all memory accesses based on the iterator variable *potentially* open > the chance for spectre [2] gadgets. The proposed mitigation was setting the iterator variable > to NULL when the terminating condition is reached (in speculative safe way). Then, > the additional speculative list iteration would still execute but won't access any > potential secret data. > > And this would also be required for list_for_each() since combined with the list_entry() > within the loop it basically is semantically identical to list_for_each_entry() > for the additional speculative iteration. > > Now, I have no strong opinion on going all the way and since 2) is not the main motivation > for this I'm also fine with sticking to your proposed solution, but it would mean that implementing > a "speculative safe" list_for_each() will be more difficult in the future since it is using > the iterator of list_for_each() past the loop. > > I hope this explains the background a bit better. > >> >>> What do you think about doing it this way: >>> >>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c >>> index b7e95d60a6e4..f5b0502c1098 100644 >>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c >>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c >>> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, >>> list_add(&e->list, &gating_cfg->entries); >>> } else { >>> struct sja1105_gate_entry *p; >>> + struct list_head *pos = NULL; >>> >>> list_for_each_entry(p, &gating_cfg->entries, list) { >>> if (p->interval == e->interval) { >>> @@ -37,10 +38,14 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, >>> goto err; >>> } >>> >>> - if (e->interval < p->interval) >>> + if (e->interval < p->interval) { >>> + pos = &p->list; >>> break; >>> + } >>> } >>> - list_add(&e->list, p->list.prev); >>> + if (!pos) >>> + pos = &gating_cfg->entries; >>> + list_add(&e->list, pos->prev); >>> } >>> >>> gating_cfg->num_entries++; >>> -- >>> >>>> >>>> Thanks for the suggestion. >>>> >>>>> } >>>>> >>>>> gating_cfg->num_entries++; >>>>> -----------------------------[ cut here ]----------------------------- >>>> >>>> [1] https://lore.kernel.org/linux-kernel/20220407102900.3086255-12-jakobkoschel@gmail.com/ >>>> >>>> Jakob >>> >>> Thanks, >>> Jakob > > Thanks, > Jakob > > [1] https://www.vusec.net/projects/kasper/ > [2] https://spectreattack.com/spectre.pdf Btw, I just realized that the if (!pos) is not necessary. This should simply do it: diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c index b7e95d60a6e4..2d59e75a9e3d 100644 --- a/drivers/net/dsa/sja1105/sja1105_vl.c +++ b/drivers/net/dsa/sja1105/sja1105_vl.c @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, list_add(&e->list, &gating_cfg->entries); } else { + struct list_head *pos = &gating_cfg->entries; struct sja1105_gate_entry *p; list_for_each_entry(p, &gating_cfg->entries, list) { if (p->interval == e->interval) { @@ -37,10 +38,12 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, goto err; } - if (e->interval < p->interval) + if (e->interval < p->interval) { + pos = &p->list; break; + } } - list_add(&e->list, p->list.prev); + list_add(&e->list, pos->prev); } gating_cfg->num_entries++; -- 2.25.1 Thanks, Jakob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel