| Message ID | 20230515085046.4457-2-jnixdorf-oss@avm.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/2] bridge: Add a limit on FDB entries | expand |
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 <jnixdorf-oss@avm.de> > --- > 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 <razor@blackwall.org>
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 <jnixdorf-oss@avm.de> > > --- > > 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 <razor@blackwall.org> 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)
On Mon, 15 May 2023 10:50:46 +0200 Johannes Nixdorf <jnixdorf-oss@avm.de> 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
Hi Johannes,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Nixdorf/bridge-Add-a-sysctl-to-limit-new-brides-FDB-entries/20230515-170206
base: net-next/main
patch link: https://lore.kernel.org/r/20230515085046.4457-2-jnixdorf-oss%40avm.de
patch subject: [PATCH net-next 2/2] bridge: Add a sysctl to limit new brides FDB entries
config: parisc-buildonly-randconfig-r001-20230515
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fd0a1c15d8c8c81217317017fae8fb99bde416fe
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johannes-Nixdorf/bridge-Add-a-sysctl-to-limit-new-brides-FDB-entries/20230515-170206
git checkout fd0a1c15d8c8c81217317017fae8fb99bde416fe
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305160725.ukBLckmK-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "br_fdb_max_entries_default" [net/bridge/bridge.ko] undefined!
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 <jnixdorf-oss@avm.de> >>> --- >>> 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 <razor@blackwall.org> > > 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.
On Mon, May 15, 2023 at 08:56:27AM -0700, Stephen Hemminger wrote: > On Mon, 15 May 2023 10:50:46 +0200 > Johannes Nixdorf <jnixdorf-oss@avm.de> 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-oss@avm.de/T/#ma4145398516bfd39dfa09976b7892f5fdb76f8c0
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 <linux/init.h> #include <linux/llc.h> #include <net/llc.h> +#include <net/netns/generic.h> #include <net/stp.h> #include <net/switchdev.h> @@ -348,6 +349,82 @@ void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on) clear_bit(opt, &br->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, &init_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 = &brnet->fdb_max_entries_default; + brnet->ctl_hdr = register_net_sysctl(net, "net/bridge", table); + if (!brnet->ctl_hdr) { + if (!net_eq(net, &init_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, &init_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 = &br_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) +{ + return 0; +} +#endif #endif
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 <jnixdorf-oss@avm.de> --- net/bridge/br.c | 83 +++++++++++++++++++++++++++++++++++++++++ net/bridge/br_device.c | 4 +- net/bridge/br_private.h | 9 +++++ 3 files changed, 95 insertions(+), 1 deletion(-)