Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero

[Nikolay Aleksandrov <nikolay@nvidia.com>]

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@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@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