Hangbin Liu
2021-Oct-18 08:26 UTC
[Bridge] [PATCH net] net: bridge: mcast: Do not allow users to set IGMP counter/timer to zero
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(-)
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
Nikolay Aleksandrov
2021-Oct-18 10:28 UTC
[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