| Message ID | 20211227172116.320768-2-nikolay@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 99b40610956a8a8755653a67392e2a8b772453be |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: mcast: add and enforce query interval minimum | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 20 this patch: 20 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 61 lines checked |
| netdev/kdoc | success | Errors and warnings before: 3 this patch: 3 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Dec 27, 2021 at 07:21:15PM +0200, Nikolay Aleksandrov wrote: > As reported[1] if query interval is set too low and we have multiple > bridges or even a single bridge with multiple querier vlans configured > we can crash the machine. Add a 1 second minimum which must be enforced > by overwriting the value if set lower (i.e. without returning an error) to > avoid breaking user-space. If that happens a log message is emitted to let > the administrator know that the interval has been set to the minimum. > The issue has been present since these intervals could be user-controlled. > > [1] https://lore.kernel.org/netdev/e8b9ce41-57b9-b6e2-a46a-ff9c791cf0ba@gmail.com/ > > Fixes: d902eee43f19 ("bridge: Add multicast count/interval sysfs entries") > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com> > --- > net/bridge/br_multicast.c | 16 ++++++++++++++++ > net/bridge/br_netlink.c | 2 +- > net/bridge/br_private.h | 3 +++ > net/bridge/br_sysfs_br.c | 2 +- > net/bridge/br_vlan_options.c | 2 +- > 5 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > index f3d751105343..998da4a2d209 100644 > --- a/net/bridge/br_multicast.c > +++ b/net/bridge/br_multicast.c > @@ -4522,6 +4522,22 @@ int br_multicast_set_mld_version(struct net_bridge_mcast *brmctx, > } > #endif > > +void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx, > + unsigned long val) > +{ > + unsigned long intvl_jiffies = clock_t_to_jiffies(val); > + > + if (intvl_jiffies < BR_MULTICAST_QUERY_INTVL_MIN) { > + br_info(brmctx->br, > + "trying to set multicast query interval below minimum, setting to %lu (%ums)\n", > + jiffies_to_clock_t(BR_MULTICAST_QUERY_INTVL_MIN), > + jiffies_to_msecs(BR_MULTICAST_QUERY_INTVL_MIN)); > + intvl_jiffies = BR_MULTICAST_QUERY_INTVL_MIN; > + } > + > + brmctx->multicast_query_interval = intvl_jiffies; > +} > + > /** > * br_multicast_list_adjacent - Returns snooped multicast addresses > * @dev: The bridge port adjacent to which to retrieve addresses > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 0c8b5f1a15bc..701dd8b8455e 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -1357,7 +1357,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[], > 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); > + br_multicast_set_query_intvl(&br->multicast_ctx, val); > } > > if (data[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]) { > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index c0efd697865a..4ed7f11042e8 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -28,6 +28,7 @@ > #define BR_MAX_PORTS (1<<BR_PORT_BITS) > > #define BR_MULTICAST_DEFAULT_HASH_MAX 4096 > +#define BR_MULTICAST_QUERY_INTVL_MIN msecs_to_jiffies(1000) > > #define BR_HWDOM_MAX BITS_PER_LONG > > @@ -963,6 +964,8 @@ int br_multicast_dump_querier_state(struct sk_buff *skb, > int nest_attr); > size_t br_multicast_querier_state_size(void); > size_t br_rports_size(const struct net_bridge_mcast *brmctx); > +void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx, > + unsigned long val); > > static inline bool br_group_is_l2(const struct br_ip *group) > { > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c > index d9a89ddd0331..f5bd1114a434 100644 > --- a/net/bridge/br_sysfs_br.c > +++ b/net/bridge/br_sysfs_br.c > @@ -658,7 +658,7 @@ 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); > + br_multicast_set_query_intvl(&br->multicast_ctx, val); > return 0; > } > > diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c > index 8ffd4ed2563c..bf1ac0874279 100644 > --- a/net/bridge/br_vlan_options.c > +++ b/net/bridge/br_vlan_options.c > @@ -521,7 +521,7 @@ static int br_vlan_process_global_one_opts(const struct net_bridge *br, > u64 val; > > val = nla_get_u64(tb[BRIDGE_VLANDB_GOPTS_MCAST_QUERY_INTVL]); > - v->br_mcast_ctx.multicast_query_interval = clock_t_to_jiffies(val); > + br_multicast_set_query_intvl(&v->br_mcast_ctx, val); > *changed = true; > } > if (tb[BRIDGE_VLANDB_GOPTS_MCAST_QUERY_RESPONSE_INTVL]) { > -- > 2.33.1 > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter>
On 12/27/21 09:21, Nikolay Aleksandrov wrote: > As reported[1] if query interval is set too low and we have multiple > bridges or even a single bridge with multiple querier vlans configured > we can crash the machine. Add a 1 second minimum which must be enforced > by overwriting the value if set lower (i.e. without returning an error) to > avoid breaking user-space. If that happens a log message is emitted to let > the administrator know that the interval has been set to the minimum. > The issue has been present since these intervals could be user-controlled. > > [1] https://lore.kernel.org/netdev/e8b9ce41-57b9-b6e2-a46a-ff9c791cf0ba@gmail.com/ > > Fixes: d902eee43f19 ("bridge: Add multicast count/interval sysfs entries") > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com> > --- Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks !
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index f3d751105343..998da4a2d209 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -4522,6 +4522,22 @@ int br_multicast_set_mld_version(struct net_bridge_mcast *brmctx, } #endif +void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx, + unsigned long val) +{ + unsigned long intvl_jiffies = clock_t_to_jiffies(val); + + if (intvl_jiffies < BR_MULTICAST_QUERY_INTVL_MIN) { + br_info(brmctx->br, + "trying to set multicast query interval below minimum, setting to %lu (%ums)\n", + jiffies_to_clock_t(BR_MULTICAST_QUERY_INTVL_MIN), + jiffies_to_msecs(BR_MULTICAST_QUERY_INTVL_MIN)); + intvl_jiffies = BR_MULTICAST_QUERY_INTVL_MIN; + } + + brmctx->multicast_query_interval = intvl_jiffies; +} + /** * br_multicast_list_adjacent - Returns snooped multicast addresses * @dev: The bridge port adjacent to which to retrieve addresses diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 0c8b5f1a15bc..701dd8b8455e 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -1357,7 +1357,7 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[], 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); + br_multicast_set_query_intvl(&br->multicast_ctx, val); } if (data[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]) { diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index c0efd697865a..4ed7f11042e8 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -28,6 +28,7 @@ #define BR_MAX_PORTS (1<<BR_PORT_BITS) #define BR_MULTICAST_DEFAULT_HASH_MAX 4096 +#define BR_MULTICAST_QUERY_INTVL_MIN msecs_to_jiffies(1000) #define BR_HWDOM_MAX BITS_PER_LONG @@ -963,6 +964,8 @@ int br_multicast_dump_querier_state(struct sk_buff *skb, int nest_attr); size_t br_multicast_querier_state_size(void); size_t br_rports_size(const struct net_bridge_mcast *brmctx); +void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx, + unsigned long val); static inline bool br_group_is_l2(const struct br_ip *group) { diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c index d9a89ddd0331..f5bd1114a434 100644 --- a/net/bridge/br_sysfs_br.c +++ b/net/bridge/br_sysfs_br.c @@ -658,7 +658,7 @@ 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); + br_multicast_set_query_intvl(&br->multicast_ctx, val); return 0; } diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c index 8ffd4ed2563c..bf1ac0874279 100644 --- a/net/bridge/br_vlan_options.c +++ b/net/bridge/br_vlan_options.c @@ -521,7 +521,7 @@ static int br_vlan_process_global_one_opts(const struct net_bridge *br, u64 val; val = nla_get_u64(tb[BRIDGE_VLANDB_GOPTS_MCAST_QUERY_INTVL]); - v->br_mcast_ctx.multicast_query_interval = clock_t_to_jiffies(val); + br_multicast_set_query_intvl(&v->br_mcast_ctx, val); *changed = true; } if (tb[BRIDGE_VLANDB_GOPTS_MCAST_QUERY_RESPONSE_INTVL]) {
As reported[1] if query interval is set too low and we have multiple bridges or even a single bridge with multiple querier vlans configured we can crash the machine. Add a 1 second minimum which must be enforced by overwriting the value if set lower (i.e. without returning an error) to avoid breaking user-space. If that happens a log message is emitted to let the administrator know that the interval has been set to the minimum. The issue has been present since these intervals could be user-controlled. [1] https://lore.kernel.org/netdev/e8b9ce41-57b9-b6e2-a46a-ff9c791cf0ba@gmail.com/ Fixes: d902eee43f19 ("bridge: Add multicast count/interval sysfs entries") Reported-by: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com> --- net/bridge/br_multicast.c | 16 ++++++++++++++++ net/bridge/br_netlink.c | 2 +- net/bridge/br_private.h | 3 +++ net/bridge/br_sysfs_br.c | 2 +- net/bridge/br_vlan_options.c | 2 +- 5 files changed, 22 insertions(+), 3 deletions(-)