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: <05d630bf-7fa8-4495-6345-207f133ef746@blackwall.org>
Date:   Tue, 20 Dec 2022 12:13:12 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report
Content-Language: en-US
To:     Joy Gu <jgu@purestorage.com>, bridge@lists.linux-foundation.org
Cc:     roopa@nvidia.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, joern@purestorage.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221220024807.36502-1-jgu@purestorage.com>
From:   Nikolay Aleksandrov <razor@blackwall.org>
In-Reply-To: <20221220024807.36502-1-jgu@purestorage.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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()). If you have hit a bug or
seen this with some compiler please provide more details, disassembly of the resulting
code would be best.

Thanks.