Re: [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report

On Tue, 2022-12-20 at 12:13 +0200, Nikolay Aleksandrov wrote:

On 20/12/2022 04:48, Joy Gu wrote:

quoted

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 <redacted>
Signed-off-by: Joy Gu <redacted>
---
 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