Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <367438a5296d6b43d92287289f44f0e1dfe01d1a.camel@redhat.com>
Subject: Re: [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report
From:   Paolo Abeni <pabeni@redhat.com>
To:     Nikolay Aleksandrov <razor@blackwall.org>,
        Joy Gu <jgu@purestorage.com>, bridge@lists.linux-foundation.org
Cc:     roopa@nvidia.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, joern@purestorage.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Tue, 20 Dec 2022 15:59:15 +0100
In-Reply-To: <05d630bf-7fa8-4495-6345-207f133ef746@blackwall.org>
References: <20221220024807.36502-1-jgu@purestorage.com>
         <05d630bf-7fa8-4495-6345-207f133ef746@blackwall.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2022-12-20 at 12:13 +0200, Nikolay Aleksandrov wrote:
> On 20/12/2022 04:48, Joy Gu wrote:
> > In br_ip4_multicast_igmp3_report() and br_ip6_multicast_mld2_report(),
> > "ih" or "mld2r" is a pointer into the skb header. It's dereferenced to
> > get "num", which is used in the for-loop condition that follows.
> > 
> > Compilers are free to not spend a register on "num" and dereference that
> > pointer every time "num" would be used, i.e. every loop iteration. Which
> > would be a bug if pskb_may_pull() (called by ip_mc_may_pull() or
> > ipv6_mc_may_pull() in the loop body) were to change pointers pointing
> > into the skb header, e.g. by freeing "skb->head".
> > 
> > We can avoid this by using READ_ONCE().
> > 
> > Suggested-by: Joern Engel <joern@purestorage.com>
> > Signed-off-by: Joy Gu <jgu@purestorage.com>
> > ---
> >  net/bridge/br_multicast.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> 
> I doubt any compiler would do that (partly due to the ntohs()). 

I would say that any compiler behaving as described above is buggy, as
'skb' is modified inside the loop, and the header pointer is fetched
from the skb, it can't be considered invariant.

> If you have hit a bug or
> seen this with some compiler please provide more details, disassembly of the resulting
> code would be best.

Exactly. A more detailed description of the issue you see is necessary.
And very likely the proposed solution is not the correct one.

Cheers,

Paolo