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

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

On 19/10/2021 08:43, Hangbin Liu wrote:
> 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 problem is not if there is meaning, we cannot start restricting option
values now after
they've become uapi (and have been for a very long time) because we can
break user-space even
though chances are pretty low. I don't think this patch is acceptable, I
commented on the other
patch issues but they don't matter because of this.

Thanks,
 Nik