Re: [Bridge] [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
On Mon, May 15, 2023 at 08:56:27AM -0700, Stephen Hemminger wrote:
> On Mon, 15 May 2023 10:50:46 +0200
> Johannes Nixdorf wrote:
>
> > +static struct ctl_table br_sysctl_table[] = {
> > + {
> > + .procname = "bridge-fdb-max-entries-default",
>
>
> That name is too long.
>
> Also, all the rest of bridge code does not use sysctl's.
The code in net/bridge/br_netfilter_hooks.c also uses sysctls, which is
where I took inspiration for the approach for setting them up, and also
the naming scheme.
> Why is this special and why should the property be global and not per bridge?
As explained in the commit message and [1] it is a global default
setting. It makes no sense to make it per bridge, as there is already
a per bridge netlink setting that overrides it. The only alternative
option is to not have it at all, which is what I will be going to do
with a v2.
> NAK
Fair enough. I took it out in my pending-v2-state of the series, but
would welcome some input on whether you see any value in the proposed
alternatives in [1], or are strictly against having a global default !=
0 here at all.
[1]:
https://lore.kernel.org/netdev/20230515085046.4457-1-jnixdorf-...@avm.de/T/#ma4145398516bfd39dfa09976b7892f5fdb76f8c0
Re: [Bridge] [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
On Mon, May 15, 2023 at 12:35:47PM +0300, Nikolay Aleksandrov wrote: > On 15/05/2023 11:50, Johannes Nixdorf wrote: > > This is a convenience setting, which allows the administrator to limit > > the default limit of FDB entries for all created bridges, instead of > > having to set it for each created bridge using the netlink property. > > > > The setting is network namespace local, and defaults to 0, which means > > unlimited, for backwards compatibility reasons. > > > > Signed-off-by: Johannes Nixdorf > > --- > > net/bridge/br.c | 83 + > > net/bridge/br_device.c | 4 +- > > net/bridge/br_private.h | 9 + > > 3 files changed, 95 insertions(+), 1 deletion(-) > > > > The bridge doesn't need private sysctls. Netlink is enough. > Nacked-by: Nikolay Aleksandrov Fair enough. I originally included the setting so there is a global setting an administrator could toggle instead of having to hunt down each process that might create a bridge, and teaching them to create them with an FDB limit. Does any of the following alternatives sound acceptable to you?: - Having the default limit (instead of the proposed default to unlimited) configurable in Kbuild. This would solve our problem, as we build our kernels ourselves, but I don't know whether putting a limit there would be acceptable for e.g. distributions. - Hardcoding a default limit != 0. I was afraid I'd break someones use-case with far too large bridged networks if I don't default to unlimited, but if you maintainers have a number in mind with which you don't see a problem, I'd be fine with it as well. (Sorry for sending this mail twice, I accidentally dropped the list and CC on the fist try)
[Bridge] [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
This is a convenience setting, which allows the administrator to limit
the default limit of FDB entries for all created bridges, instead of
having to set it for each created bridge using the netlink property.
The setting is network namespace local, and defaults to 0, which means
unlimited, for backwards compatibility reasons.
Signed-off-by: Johannes Nixdorf
---
net/bridge/br.c | 83 +
net/bridge/br_device.c | 4 +-
net/bridge/br_private.h | 9 +
3 files changed, 95 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br.c b/net/bridge/br.c
index 4f5098d33a46..e32bb956111c 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -348,6 +349,82 @@ void br_opt_toggle(struct net_bridge *br, enum
net_bridge_opts opt, bool on)
clear_bit(opt, >options);
}
+#ifdef CONFIG_SYSCTL
+static unsigned int br_net_id __read_mostly;
+
+struct br_net {
+ struct ctl_table_header *ctl_hdr;
+
+ unsigned int fdb_max_entries_default;
+};
+
+static int br_proc_rtnl_uintvec(struct ctl_table *table, int write,
+ void *buffer, size_t *lenp, loff_t *ppos)
+{
+ int ret;
+
+ rtnl_lock();
+ ret = proc_douintvec(table, write, buffer, lenp, ppos);
+ rtnl_unlock();
+
+ return ret;
+}
+
+static struct ctl_table br_sysctl_table[] = {
+ {
+ .procname = "bridge-fdb-max-entries-default",
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler = br_proc_rtnl_uintvec,
+ },
+ { }
+};
+
+static int __net_init br_net_init(struct net *net)
+{
+ struct ctl_table *table = br_sysctl_table;
+ struct br_net *brnet;
+
+ if (!net_eq(net, _net)) {
+ table = kmemdup(table, sizeof(br_sysctl_table), GFP_KERNEL);
+ if (!table)
+ return -ENOMEM;
+ }
+
+ brnet = net_generic(net, br_net_id);
+
+ brnet->fdb_max_entries_default = 0;
+
+ table[0].data = >fdb_max_entries_default;
+ brnet->ctl_hdr = register_net_sysctl(net, "net/bridge", table);
+ if (!brnet->ctl_hdr) {
+ if (!net_eq(net, _net))
+ kfree(table);
+
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void __net_exit br_net_exit(struct net *net)
+{
+ struct br_net *brnet = net_generic(net, br_net_id);
+ struct ctl_table *table = brnet->ctl_hdr->ctl_table_arg;
+
+ unregister_net_sysctl_table(brnet->ctl_hdr);
+ if (!net_eq(net, _net))
+ kfree(table);
+}
+
+unsigned int br_fdb_max_entries_default(struct net *net)
+{
+ struct br_net *brnet = net_generic(net, br_net_id);
+
+ return brnet->fdb_max_entries_default;
+}
+#endif
+
static void __net_exit br_net_exit_batch(struct list_head *net_list)
{
struct net_device *dev;
@@ -367,6 +444,12 @@ static void __net_exit br_net_exit_batch(struct list_head
*net_list)
}
static struct pernet_operations br_net_ops = {
+#ifdef CONFIG_SYSCTL
+ .init = br_net_init,
+ .exit = br_net_exit,
+ .id = _net_id,
+ .size = sizeof(struct br_net),
+#endif
.exit_batch = br_net_exit_batch,
};
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index d455a28df7c9..26023f2732e8 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -117,8 +117,11 @@ static void br_set_lockdep_class(struct net_device *dev)
static int br_dev_init(struct net_device *dev)
{
struct net_bridge *br = netdev_priv(dev);
+ struct net *net = dev_net(dev);
int err;
+ br->fdb_max_entries = br_fdb_max_entries_default(net);
+
dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
if (!dev->tstats)
return -ENOMEM;
@@ -529,7 +532,6 @@ void br_dev_setup(struct net_device *dev)
br->bridge_forward_delay = br->forward_delay = 15 * HZ;
br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
br->fdb_n_entries = 0;
- br->fdb_max_entries = 0;
dev->max_mtu = ETH_MAX_MTU;
br_netfilter_rtable_init(br);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 64fb359c6e3e..d4b0f85cc278 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -2223,4 +2223,13 @@ void br_do_suppress_nd(struct sk_buff *skb, struct
net_bridge *br,
u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb, struct nd_msg *m);
bool br_is_neigh_suppress_enabled(const struct net_bridge_port *p, u16 vid);
+
+#ifdef CONFIG_SYSFS
+unsigned int br_fdb_max_entries_default(struct net *net);
+#else
+static inline unsigned int br_fdb_max_entries_default(struct net *net)
+{
+
Re: [Bridge] [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
On 15/05/2023 14:27, Johannes Nixdorf wrote: > On Mon, May 15, 2023 at 12:35:47PM +0300, Nikolay Aleksandrov wrote: >> On 15/05/2023 11:50, Johannes Nixdorf wrote: >>> This is a convenience setting, which allows the administrator to limit >>> the default limit of FDB entries for all created bridges, instead of >>> having to set it for each created bridge using the netlink property. >>> >>> The setting is network namespace local, and defaults to 0, which means >>> unlimited, for backwards compatibility reasons. >>> >>> Signed-off-by: Johannes Nixdorf >>> --- >>> net/bridge/br.c | 83 + >>> net/bridge/br_device.c | 4 +- >>> net/bridge/br_private.h | 9 + >>> 3 files changed, 95 insertions(+), 1 deletion(-) >>> >> >> The bridge doesn't need private sysctls. Netlink is enough. >> Nacked-by: Nikolay Aleksandrov > > Fair enough. > > I originally included the setting so there is a global setting an > administrator could toggle instead of having to hunt down each process > that might create a bridge, and teaching them to create them with an > FDB limit. > > Does any of the following alternatives sound acceptable to you?: > - Having the default limit (instead of the proposed default to unlimited) >configurable in Kbuild. This would solve our problem, as we build >our kernels ourselves, but I don't know whether putting a limit there >would be acceptable for e.g. distributions. I don't mind, but it would be useless for everyone else. Kernels would be built without that limit set. > - Hardcoding a default limit != 0. I was afraid I'd break someones >use-case with far too large bridged networks if I don't default to >unlimited, but if you maintainers have a number in mind with which >you don't see a problem, I'd be fine with it as well. > > (Sorry for sending this mail twice, I accidentally dropped the list and > CC on the fist try) Right, that has been discussed before. So far there hasn't been any good option, so I'd say for the time being (or unless you have some better idea) we should stick with the netlink max attribute and distributions/admins would have to set it on bridge creation. We could add a warning when creating a bridge without fdb limit to remind people that it's advisable to set it. That warning can be removed when we come up with a proper solution.
Re: [Bridge] [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
On Mon, 15 May 2023 10:50:46 +0200
Johannes Nixdorf wrote:
> +static struct ctl_table br_sysctl_table[] = {
> + {
> + .procname = "bridge-fdb-max-entries-default",
That name is too long.
Also, all the rest of bridge code does not use sysctl's. Why is this
special and why should the property be global and not per bridge?
NAK
Re: [Bridge] [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
On 15/05/2023 11:50, Johannes Nixdorf wrote: > This is a convenience setting, which allows the administrator to limit > the default limit of FDB entries for all created bridges, instead of > having to set it for each created bridge using the netlink property. > > The setting is network namespace local, and defaults to 0, which means > unlimited, for backwards compatibility reasons. > > Signed-off-by: Johannes Nixdorf > --- > net/bridge/br.c | 83 + > net/bridge/br_device.c | 4 +- > net/bridge/br_private.h | 9 + > 3 files changed, 95 insertions(+), 1 deletion(-) > The bridge doesn't need private sysctls. Netlink is enough. Nacked-by: Nikolay Aleksandrov
