Re: [v2,net-next,02/15] ipmr: Annotate access to mrt->mroute_do_{pim,assert,wrvifwhole}.

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ipmr: Annotate access to mrt->mroute_do_{pim,assert,wrvifwhole}.

This commit adds READ_ONCE() and WRITE_ONCE() annotations to protect
lockless access to three fields in struct mr_table: mroute_do_pim,
mroute_do_assert, and mroute_do_wrvifwhole.

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 131382c388e9..377b5848f99c 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c

[ ... ]

> @@ -2037,7 +2037,7 @@ static void ip_mr_forward(struct net *net, struct mr_table *mrt,
>
>  		atomic_long_inc(&c->_c.mfc_un.res.wrong_if);
>
> -		if (true_vifi >= 0 && mrt->mroute_do_assert &&
> +		if (true_vifi >= 0 && READ_ONCE(mrt->mroute_do_assert) &&
>  		    /* pimsm uses asserts, when switching from RPT to SPT,
>  		     * so that we cannot check that packet arrived on an oif.
>  		     * It is bad, but otherwise we would need to move pretty
> @@ -2050,7 +2050,7 @@ static void ip_mr_forward(struct net *net, struct mr_table *mrt,
>  			       MFC_ASSERT_THRESH)) {
>  			c->_c.mfc_un.res.last_assert = jiffies;
>  			ipmr_cache_report(mrt, skb, true_vifi, IGMPMSG_WRONGVIF);
> -			if (mrt->mroute_do_wrvifwhole)
> +			if (READ_ONCE(mrt->mroute_do_wrvifwhole))
>  				ipmr_cache_report(mrt, skb, true_vifi,
>  						  IGMPMSG_WRVIFWHOLE);
>  		}

Should the condition at the start of this block also use READ_ONCE() for
mrt->mroute_do_pim? The check reads:

		if (true_vifi >= 0 && READ_ONCE(mrt->mroute_do_assert) &&
		    (mrt->mroute_do_pim ||
		     c->_c.mfc_un.res.ttls[true_vifi] < 255) &&

The patch adds WRITE_ONCE() for mrt->mroute_do_pim on the writer side in
ip_mroute_setsockopt() and READ_ONCE() for it on the reader side in
ip_mroute_getsockopt(), but the lockless read of mrt->mroute_do_pim in
this condition is not annotated.

Since ip_mr_forward() runs under rcu_read_lock() (not RTNL), this read is
lockless and needs READ_ONCE() to pair with the WRITE_ONCE() added on the
writer side. The adjacent reads of mrt->mroute_do_assert and
mrt->mroute_do_wrvifwhole in the same function were correctly annotated
with READ_ONCE() by this patch, making the omission of mrt->mroute_do_pim
inconsistent.