All of lore.kernel.org
 help / color / mirror / Atom feed
* [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
@ 2021-10-18  8:26 Hangbin Liu
  2021-10-18 10:28 ` Nikolay Aleksandrov
  0 siblings, 1 reply; 7+ messages in thread
From: Hangbin Liu @ 2021-10-18  8:26 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: bridge, Nikolay Aleksandrov, Hangbin Liu, roopa, kuba, davem

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(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 5c6c4305ed23..d054936373ac 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1168,6 +1168,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 			 struct netlink_ext_ack *extack)
 {
 	struct net_bridge *br = netdev_priv(brdev);
+	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
 	int err;
 
 	if (!data)
@@ -1287,8 +1288,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 	if (data[IFLA_BR_MCAST_ROUTER]) {
 		u8 multicast_router = nla_get_u8(data[IFLA_BR_MCAST_ROUTER]);
 
-		err = br_multicast_set_router(&br->multicast_ctx,
-					      multicast_router);
+		err = br_multicast_set_router(brmctx, multicast_router);
 		if (err)
 			return err;
 	}
@@ -1311,8 +1311,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 	if (data[IFLA_BR_MCAST_QUERIER]) {
 		u8 mcast_querier = nla_get_u8(data[IFLA_BR_MCAST_QUERIER]);
 
-		err = br_multicast_set_querier(&br->multicast_ctx,
-					       mcast_querier);
+		err = br_multicast_set_querier(brmctx, mcast_querier);
 		if (err)
 			return err;
 	}
@@ -1327,49 +1326,93 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 	if (data[IFLA_BR_MCAST_LAST_MEMBER_CNT]) {
 		u32 val = nla_get_u32(data[IFLA_BR_MCAST_LAST_MEMBER_CNT]);
 
-		br->multicast_ctx.multicast_last_member_count = val;
+		if (val) {
+			brmctx->multicast_last_member_count = val;
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Last Member Count");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_STARTUP_QUERY_CNT]) {
 		u32 val = nla_get_u32(data[IFLA_BR_MCAST_STARTUP_QUERY_CNT]);
 
-		br->multicast_ctx.multicast_startup_query_count = val;
+		if (val) {
+			brmctx->multicast_startup_query_count = val;
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Startup Query Count");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_LAST_MEMBER_INTVL]) {
 		u64 val = nla_get_u64(data[IFLA_BR_MCAST_LAST_MEMBER_INTVL]);
 
-		br->multicast_ctx.multicast_last_member_interval = clock_t_to_jiffies(val);
+		if (val) {
+			brmctx->multicast_last_member_interval = clock_t_to_jiffies(val);
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Last Member Interval");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_MEMBERSHIP_INTVL]) {
 		u64 val = nla_get_u64(data[IFLA_BR_MCAST_MEMBERSHIP_INTVL]);
 
-		br->multicast_ctx.multicast_membership_interval = clock_t_to_jiffies(val);
+		if (val) {
+			brmctx->multicast_membership_interval = clock_t_to_jiffies(val);
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Membership Interval");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_QUERIER_INTVL]) {
 		u64 val = nla_get_u64(data[IFLA_BR_MCAST_QUERIER_INTVL]);
 
-		br->multicast_ctx.multicast_querier_interval = clock_t_to_jiffies(val);
+		if (val) {
+			brmctx->multicast_querier_interval = clock_t_to_jiffies(val);
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Querier Interval");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_QUERY_INTVL]) {
 		u64 val = nla_get_u64(data[IFLA_BR_MCAST_QUERY_INTVL]);
 
-		br->multicast_ctx.multicast_query_interval = clock_t_to_jiffies(val);
+		if (val && clock_t_to_jiffies(val) > brmctx->multicast_query_response_interval) {
+			brmctx->multicast_query_interval = clock_t_to_jiffies(val);
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Query Interval");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]) {
 		u64 val = nla_get_u64(data[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]);
 
-		br->multicast_ctx.multicast_query_response_interval = clock_t_to_jiffies(val);
+		/* RFC3376 8.3: The number of seconds represented by the
+		 * [Query Response Interval] must be less than the [Query
+		 * Interval].
+		 */
+		if (val && clock_t_to_jiffies(val) < brmctx->multicast_query_interval) {
+			brmctx->multicast_query_response_interval = clock_t_to_jiffies(val);
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Query Response Interval");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]) {
 		u64 val = nla_get_u64(data[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]);
 
-		br->multicast_ctx.multicast_startup_query_interval = clock_t_to_jiffies(val);
+		if (val) {
+			brmctx->multicast_startup_query_interval = clock_t_to_jiffies(val);
+		} else {
+			NL_SET_ERR_MSG(extack, "Invalid Startup Query Interval");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_BR_MCAST_STATS_ENABLED]) {
@@ -1383,8 +1426,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 		__u8 igmp_version;
 
 		igmp_version = nla_get_u8(data[IFLA_BR_MCAST_IGMP_VERSION]);
-		err = br_multicast_set_igmp_version(&br->multicast_ctx,
-						    igmp_version);
+		err = br_multicast_set_igmp_version(brmctx, igmp_version);
 		if (err)
 			return err;
 	}
@@ -1394,8 +1436,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 		__u8 mld_version;
 
 		mld_version = nla_get_u8(data[IFLA_BR_MCAST_MLD_VERSION]);
-		err = br_multicast_set_mld_version(&br->multicast_ctx,
-						   mld_version);
+		err = br_multicast_set_mld_version(brmctx, mld_version);
 		if (err)
 			return err;
 	}
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index d9a89ddd0331..e311aa651d03 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -542,8 +542,13 @@ static ssize_t multicast_last_member_count_show(struct device *d,
 static int set_last_member_count(struct net_bridge *br, unsigned long val,
 				 struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_last_member_count = val;
-	return 0;
+	if (val) {
+		br->multicast_ctx.multicast_last_member_count = val;
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Last Member Count");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_last_member_count_store(struct device *d,
@@ -564,8 +569,13 @@ static ssize_t multicast_startup_query_count_show(
 static int set_startup_query_count(struct net_bridge *br, unsigned long val,
 				   struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_startup_query_count = val;
-	return 0;
+	if (val) {
+		br->multicast_ctx.multicast_startup_query_count = val;
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Startup Query Count");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_startup_query_count_store(
@@ -587,8 +597,13 @@ static ssize_t multicast_last_member_interval_show(
 static int set_last_member_interval(struct net_bridge *br, unsigned long val,
 				    struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_last_member_interval = clock_t_to_jiffies(val);
-	return 0;
+	if (val) {
+		br->multicast_ctx.multicast_last_member_interval = clock_t_to_jiffies(val);
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Last Member Interval");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_last_member_interval_store(
@@ -610,8 +625,13 @@ static ssize_t multicast_membership_interval_show(
 static int set_membership_interval(struct net_bridge *br, unsigned long val,
 				   struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_membership_interval = clock_t_to_jiffies(val);
-	return 0;
+	if (val) {
+		br->multicast_ctx.multicast_membership_interval = clock_t_to_jiffies(val);
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Membership Interval");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_membership_interval_store(
@@ -634,8 +654,13 @@ static ssize_t multicast_querier_interval_show(struct device *d,
 static int set_querier_interval(struct net_bridge *br, unsigned long val,
 				struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_querier_interval = clock_t_to_jiffies(val);
-	return 0;
+	if (val) {
+		br->multicast_ctx.multicast_querier_interval = clock_t_to_jiffies(val);
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Querier Interval");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_querier_interval_store(struct device *d,
@@ -658,8 +683,13 @@ static ssize_t multicast_query_interval_show(struct device *d,
 static int set_query_interval(struct net_bridge *br, unsigned long val,
 			      struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_query_interval = clock_t_to_jiffies(val);
-	return 0;
+	if (val && clock_t_to_jiffies(val) > br->multicast_ctx.multicast_query_response_interval) {
+		br->multicast_ctx.multicast_query_interval = clock_t_to_jiffies(val);
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Query Interval");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_query_interval_store(struct device *d,
@@ -682,8 +712,16 @@ static ssize_t multicast_query_response_interval_show(
 static int set_query_response_interval(struct net_bridge *br, unsigned long val,
 				       struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_query_response_interval = clock_t_to_jiffies(val);
-	return 0;
+	/* RFC3376 8.3: The number of seconds represented by the
+	 * [Query Response Interval] must be less than the [Query Interval].
+	 */
+	if (val && clock_t_to_jiffies(val) < br->multicast_ctx.multicast_query_interval) {
+		br->multicast_ctx.multicast_query_response_interval = clock_t_to_jiffies(val);
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Query Response Interval");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_query_response_interval_store(
@@ -706,8 +744,13 @@ static ssize_t multicast_startup_query_interval_show(
 static int set_startup_query_interval(struct net_bridge *br, unsigned long val,
 				      struct netlink_ext_ack *extack)
 {
-	br->multicast_ctx.multicast_startup_query_interval = clock_t_to_jiffies(val);
-	return 0;
+	if (val) {
+		br->multicast_ctx.multicast_startup_query_interval = clock_t_to_jiffies(val);
+		return 0;
+	} else {
+		NL_SET_ERR_MSG(extack, "Invalid Startup Query Interval");
+		return -EINVAL;
+	}
 }
 
 static ssize_t multicast_startup_query_interval_store(
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
  2021-10-18  8:26 [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero Hangbin Liu
@ 2021-10-18 10:28 ` Nikolay Aleksandrov
  2021-10-19  5:43   ` Hangbin Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Aleksandrov @ 2021-10-18 10:28 UTC (permalink / raw)
  To: Hangbin Liu, Nikolay Aleksandrov; +Cc: kuba, bridge, davem, roopa

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
  2021-10-18 10:28 ` Nikolay Aleksandrov
@ 2021-10-19  5:43   ` Hangbin Liu
  2021-10-19 16:09     ` Nikolay Aleksandrov
  0 siblings, 1 reply; 7+ messages in thread
From: Hangbin Liu @ 2021-10-19  5:43 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: kuba, Nikolay Aleksandrov, bridge, davem, roopa

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
  2021-10-19  5:43   ` Hangbin Liu
@ 2021-10-19 16:09     ` Nikolay Aleksandrov
  2021-10-20  1:02       ` Hangbin Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Aleksandrov @ 2021-10-19 16:09 UTC (permalink / raw)
  To: Hangbin Liu; +Cc: kuba, Nikolay Aleksandrov, bridge, davem, roopa

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
  2021-10-19 16:09     ` Nikolay Aleksandrov
@ 2021-10-20  1:02       ` Hangbin Liu
  2021-10-20 15:19         ` Stephen Hemminger
  0 siblings, 1 reply; 7+ messages in thread
From: Hangbin Liu @ 2021-10-20  1:02 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: kuba, Nikolay Aleksandrov, bridge, davem, roopa

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
  2021-10-20  1:02       ` Hangbin Liu
@ 2021-10-20 15:19         ` Stephen Hemminger
  2021-10-21  1:08           ` Hangbin Liu
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2021-10-20 15:19 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: Nikolay Aleksandrov, bridge, Nikolay Aleksandrov, roopa, kuba, davem

On Wed, 20 Oct 2021 09:02:01 +0800
Hangbin Liu <liuhangbin@gmail.com> 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
  2021-10-20 15:19         ` Stephen Hemminger
@ 2021-10-21  1:08           ` Hangbin Liu
  0 siblings, 0 replies; 7+ messages in thread
From: Hangbin Liu @ 2021-10-21  1:08 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Nikolay Aleksandrov, bridge, Nikolay Aleksandrov, roopa, kuba, davem

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-21  1:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18  8:26 [Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero Hangbin Liu
2021-10-18 10:28 ` Nikolay Aleksandrov
2021-10-19  5:43   ` Hangbin Liu
2021-10-19 16:09     ` Nikolay Aleksandrov
2021-10-20  1:02       ` Hangbin Liu
2021-10-20 15:19         ` Stephen Hemminger
2021-10-21  1:08           ` Hangbin Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.