Linux Ethernet Bridging - Oct 2021 - [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero

On 18/10/2021 11:26, Hangbin Liu wrote:
> There is no meaning to set an IGMP counter/timer to 0. Or it will cause
> unexpected behavior. E.g. if set multicast_membership_interval to 0,
> bridge will remove the mdb immediately after adding.
> 
> Fixes: 79b859f573d6 ("bridge: netlink: add support for
multicast_last_member_count")
> Fixes: b89e6babad4b ("bridge: netlink: add support for
multicast_startup_query_count")
> Fixes: 7e4df51eb35d ("bridge: netlink: add support for igmp's
intervals")
> Signed-off-by: Hangbin Liu <liuhangbin at gmail.com>
> ---
>  net/bridge/br_netlink.c  | 73 +++++++++++++++++++++++++++++---------
>  net/bridge/br_sysfs_br.c | 75 +++++++++++++++++++++++++++++++---------
>  2 files changed, 116 insertions(+), 32 deletions(-)
> 
Nacked-by: Nikolay Aleksandrov <nikolay at nvidia.com>

For a few reasons,
I'll start with the obvious that - yes, users are allowed to change the
values to non-RFC
compliant, but we cannot change that now as we'd risk breaking user-space
which is probably
doing that somewhere with some of the values below. We can fix any issues that
might arise
from doing it though, so it doesn't affect normal operation. If changing
some of the options
to 0 or to unreasonably high values lead to problems let's fix those and we
could discuss
adding constraints there if necessary.

The second issue is that you're mixing different checks below, you say do
not allow zero
but you're also checking for RFC compliance between different values.

The third issue is that you haven't done the same change for the same values
for per-vlan
multicast options (we have the same options per-vlan as well).

Your fixes tags are wrong, too. Most of these values could be set well before
they were
available through netlink.

Note on the style - generally I'd add helpers to set them and add the
constraints in those
helpers, so they can be used for both netlink and sysfs. It would definitely
target net-next
unless it's an actual bug fix.

Thanks,
 Nik

Hi Nikolay,

On Mon, Oct 18, 2021 at 01:28:14PM +0300, Nikolay Aleksandrov
wrote:
> On 18/10/2021 11:26, Hangbin Liu wrote:
> > There is no meaning to set an IGMP counter/timer to 0. Or it will
cause
> > unexpected behavior. E.g. if set multicast_membership_interval to 0,
> > bridge will remove the mdb immediately after adding.
> > 
> > Fixes: 79b859f573d6 ("bridge: netlink: add support for
multicast_last_member_count")
> > Fixes: b89e6babad4b ("bridge: netlink: add support for
multicast_startup_query_count")
> > Fixes: 7e4df51eb35d ("bridge: netlink: add support for igmp's
intervals")
> > Signed-off-by: Hangbin Liu <liuhangbin at gmail.com>
> > ---
> >  net/bridge/br_netlink.c  | 73 +++++++++++++++++++++++++++++---------
> >  net/bridge/br_sysfs_br.c | 75
+++++++++++++++++++++++++++++++---------
> >  2 files changed, 116 insertions(+), 32 deletions(-)
> > 
> 
> Nacked-by: Nikolay Aleksandrov <nikolay at nvidia.com>
> 
> For a few reasons,
> I'll start with the obvious that - yes, users are allowed to change the
values to non-RFC
> compliant, but we cannot change that now as we'd risk breaking
user-space which is probably
> doing that somewhere with some of the values below. We can fix any issues
that might arise
> from doing it though, so it doesn't affect normal operation. If
changing some of the options
> to 0 or to unreasonably high values lead to problems let's fix those
and we could discuss
> adding constraints there if necessary.
I started this patch when I saw there is not limit for setting
multicast_membership_interval to 0, which will cause bridge remove the
mdb directly after adding. Do you think this is a problem.

And what about others? I don't think there is a meaning to set other
intervals
to 0.
> 
> The second issue is that you're mixing different checks below, you say
do not allow zero
> but you're also checking for RFC compliance between different values.
Do you mean the RFC3376 8.3 rule? I can fix it in another patch.
> 
> The third issue is that you haven't done the same change for the same
values for per-vlan
> multicast options (we have the same options per-vlan as well).
Ah, thanks, I could fix that.
> 
> Your fixes tags are wrong, too. Most of these values could be set well
before they were
> available through netlink.
Oh... Then how should I set the fixes tag? Since I want fix both the netlink
configs and sys configs. Add a new one d902eee43f19 ("bridge: Add multicast
count/interval sysfs entries")
> 
> Note on the style - generally I'd add helpers to set them and add the
constraints in those
> helpers, so they can be used for both netlink and sysfs. It would
definitely target net-next
> unless it's an actual bug fix.
How about a helper like:

int br_multicast_set_interval(unsigned long *mcast_val, u64 val)
{
	if (val) {
		mcast_val = clock_t_to_jiffies(val);
		return 0;
	} else {
		NL_SET_ERR_MSG(extack, "Invalid multicast interval, should not be
0");
		return -EINVAL;
	}
}

Thanks
Hangbin