Re: [PATCH net 4/5] ipv4: restrict igmp_qrv to 255

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds an upper bound to net.ipv4.igmp_qrv.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but even with
  the new cap igmp_qrv still accepts 1..255.

--

ipv4: restrict igmp_qrv to 255

This commit caps net.ipv4.igmp_qrv at U8_MAX. Without the cap, the value
is truncated when it is stored into the u8 in_device::mr_qrv and the u8
crcount/sf_crcount counters. With a value of 256, IGMPv3 state change
records are never sent.

> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index e3760daa34706..ef631a2759b03 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c

[ ... ]

> @@ -978,7 +979,8 @@ static const struct ctl_table ipv4_net_table[] = {
>  		.maxlen		= sizeof(int),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec_minmax,
> -		.extra1		= SYSCTL_ONE
> +		.extra1		= SYSCTL_ONE,
> +		.extra2		= &igmp_qrv_max,

[Severity: Low]
Should Documentation/networking/ip-sysctl.rst be updated as well? With
this patch, a write of 256 or more to net.ipv4.igmp_qrv fails with
-EINVAL, but the igmp_qrv entry still lists only a default and a minimum:

igmp_qrv - INTEGER
	Controls the IGMP query robustness variable (see RFC2236 8.1).

	Default: 2 (as specified by RFC2236 8.1)

	Minimum: 1 (as specified by RFC6636 4.5)

Could a "Maximum: 255" line be added here?

The companion patch "ipv6: restrict mld_qrv to 255" leaves the mld_qrv
entry in the same state.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new cap still allows
values from 8 to 255. Is that enough for the AMT relay?

drivers/net/amt.c writes the sysctl straight into the 3-bit QRV field of
struct igmpv3_query (qrv:3 in include/uapi/linux/igmp.h):

drivers/net/amt.c:amt_build_igmp_gq() {
    ...
	ihv3->qrv	= READ_ONCE(amt->net->ipv4.sysctl_igmp_qrv);
    ...
}

In relay mode, amt_newlink() also copies the same sysctl into amt->qrv:

	if (amt->mode == AMT_MODE_RELAY) {
		amt->qrv = READ_ONCE(amt->net->ipv4.sysctl_igmp_qrv);

amt_build_mld_gq() then writes amt->qrv into the 3-bit MLDv2 field:

	mld2q->mld2q_qrv	= amt->qrv;

Neither path clamps the value or applies the RFC 3376 4.1.6 rule, which
says a robustness variable above 7 is sent as QRV=0.

So with igmp_qrv set to 9 or 10, does the relay send QRV=1 or QRV=2 on
the wire? If so, hosts that follow the querier's QRV would use a lower
robustness value than the one configured.

A fix for this would probably belong in amt.c rather than in the sysctl
bound.

>  	},
>  #endif
>  	{

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023091419.167088-1-jiayuan.chen%40linux.dev