Re: [Bridge] [PATCHv2 net] net: bridge: mcast: QRI must be less than QI
On 20/10/2021 05:40, Hangbin Liu wrote:
> Based on RFC3376 8.3:
> The number of seconds represented by the [Query Response Interval]
> must be less than the [Query Interval].
>
> Fixes: d902eee43f19 ("bridge: Add multicast count/interval sysfs entries")
> Signed-off-by: Hangbin Liu
> ---
> net/bridge/br_multicast.c| 27 +++
> net/bridge/br_netlink.c | 8 ++--
> net/bridge/br_private.h | 4
> net/bridge/br_sysfs_br.c | 6 ++
> net/bridge/br_vlan_options.c | 8 ++--
> 5 files changed, 45 insertions(+), 8 deletions(-)
>
Nacked-by: Nikolay Aleksandrov
I think we just discussed this a day ago? It is the same problem -
while we all agree the values should follow the RFC, users have had
the option to set any values forever (even non-RFC compliant ones).
This change risks breaking user-space.
Users are free to follow the RFC or not, we can't force them at this
point. This should've been done when the config option was added long
time ago.
Thanks,
Nik
Re: [Bridge] [PATCHv2 net] net: bridge: mcast: QRI must be less than QI
On Wed, Oct 20, 2021 at 12:49:17PM +0300, Nikolay Aleksandrov wrote: > Nacked-by: Nikolay Aleksandrov > > I think we just discussed this a day ago? It is the same problem - > while we all agree the values should follow the RFC, users have had > the option to set any values forever (even non-RFC compliant ones). > This change risks breaking user-space. OK, I misunderstood your reply in last mail. I thought you only object to disabling no meaning values(e.g. set timer to 0, which not is forbid by the RFC). I don't know you also reject to follow a *MUST* rule defined in the RFC. > > Users are free to follow the RFC or not, we can't force them at this > point. This should've been done when the config option was added long > time ago. OK, I will stop working on this path. Thanks Hangbin
Re: [Bridge] [PATCHv2 net] net: bridge: mcast: QRI must be less than QI
On Wed, Oct 20, 2021 at 06:19:25PM +0800, Hangbin Liu wrote: > On Wed, Oct 20, 2021 at 12:49:17PM +0300, Nikolay Aleksandrov wrote: > > Nacked-by: Nikolay Aleksandrov > > > > I think we just discussed this a day ago? It is the same problem - > > while we all agree the values should follow the RFC, users have had > > the option to set any values forever (even non-RFC compliant ones). > > This change risks breaking user-space. > > OK, I misunderstood your reply in last mail. I thought you only object to > disabling no meaning values(e.g. set timer to 0, which not is forbid by the > RFC). I don't know you also reject to follow a *MUST* rule defined in the RFC. I know you denied the patch due to user-space compatibility. Forgive me if my last reply sound a little aggressive. Thanks Hangbin
Re: [Bridge] [PATCHv2 net] net: bridge: mcast: QRI must be less than QI
On 20/10/2021 15:10, Hangbin Liu wrote: > On Wed, Oct 20, 2021 at 06:19:25PM +0800, Hangbin Liu wrote: >> On Wed, Oct 20, 2021 at 12:49:17PM +0300, Nikolay Aleksandrov wrote: >>> Nacked-by: Nikolay Aleksandrov >>> >>> I think we just discussed this a day ago? It is the same problem - >>> while we all agree the values should follow the RFC, users have had >>> the option to set any values forever (even non-RFC compliant ones). >>> This change risks breaking user-space. >> >> OK, I misunderstood your reply in last mail. I thought you only object to >> disabling no meaning values(e.g. set timer to 0, which not is forbid by the >> RFC). I don't know you also reject to follow a *MUST* rule defined in the >> RFC. > > I know you denied the patch due to user-space compatibility. Forgive me > if my last reply sound a little aggressive. > > Thanks > Hangbin > No worries. :) I obviously agree that it should be RFC compliant, but we must do it in a different way that doesn't risk breaking users, it goes also for how the values are computed. In the future when more of the RFC is implemented we might need to force compliance and that might require adding a new option, I guess we'll see when we get there. Cheers, Nik
Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
On Wed, 20 Oct 2021 09:02:01 +0800 Hangbin Liu wrote: > On Tue, Oct 19, 2021 at 07:09:42PM +0300, Nikolay Aleksandrov wrote: > > > 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. > > OK, I got your mean, we should not restrict the configurations based on > whether > there is a meaning. > > Thanks > Hangbin Maybe the bridge command could enforce that the value set are sane relative to the RFC? We already fixup some things in iproute2 utilities to workaround places where changing defaults in kernel would break userspace.
Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
On Wed, Oct 20, 2021 at 08:19:37AM -0700, Stephen Hemminger wrote: > > On Tue, Oct 19, 2021 at 07:09:42PM +0300, Nikolay Aleksandrov wrote: > > > > 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. > > > > OK, I got your mean, we should not restrict the configurations based on > > whether > > there is a meaning. > > > > Thanks > > Hangbin > > Maybe the bridge command could enforce that the value set are sane relative > to the RFC? We already fixup some things in iproute2 utilities to workaround > places where changing defaults in kernel would break userspace. I'm afraid this may make user more confused. As user could also echo the values via sys fs directly. e.g. # ip link set br0 type bridge mcast_query_interval 0 Error: Invalid QI, must greater than 0. # echo 0 > /sys/devices/virtual/net/br0/bridge/multicast_query_interval Then ip -d link show br0 would show the mcast_query_interval is 0. Thanks Hangbin
