From: Paolo Abeni <pabeni@redhat.com>
To: Nikolay Aleksandrov <razor@blackwall.org>,
Joy Gu <jgu@purestorage.com>,
bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
edumazet@google.com, roopa@nvidia.com, kuba@kernel.org,
davem@davemloft.net, joern@purestorage.com
Subject: Re: [Bridge] [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report
Date: Tue, 20 Dec 2022 15:59:15 +0100 [thread overview]
Message-ID: <367438a5296d6b43d92287289f44f0e1dfe01d1a.camel@redhat.com> (raw)
In-Reply-To: <05d630bf-7fa8-4495-6345-207f133ef746@blackwall.org>
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
prev parent reply other threads:[~2022-12-20 14:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-20 2:48 [Bridge] [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report Joy Gu
2022-12-20 10:13 ` Nikolay Aleksandrov
2022-12-20 14:59 ` Paolo Abeni [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=367438a5296d6b43d92287289f44f0e1dfe01d1a.camel@redhat.com \
--to=pabeni@redhat.com \
--cc=bridge@lists.linux-foundation.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jgu@purestorage.com \
--cc=joern@purestorage.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=razor@blackwall.org \
--cc=roopa@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).