Re: [Bridge] [PATCH net-next 1/2] br_netfilter: add struct netns_brnf
On Tue, Nov 27, 2018 at 01:20:47AM +0100, Pablo Neira Ayuso wrote:
> Hi,
>
> On Wed, Nov 07, 2018 at 02:48:58PM +0100, Christian Brauner wrote:
> [...]
> > diff --git a/include/net/netns/netfilter.h b/include/net/netns/netfilter.h
> > index ca043342c0eb..eedbd1ac940e 100644
> > --- a/include/net/netns/netfilter.h
> > +++ b/include/net/netns/netfilter.h
> > @@ -35,4 +35,20 @@ struct netns_nf {
> > booldefrag_ipv6;
> > #endif
> > };
> > +
> > +struct netns_brnf {
> > +#ifdef CONFIG_SYSCTL
> > + struct ctl_table_header *ctl_hdr;
> > +#endif
> > +
> > + /* default value is 1 */
> > + int call_iptables;
> > + int call_ip6tables;
> > + int call_arptables;
> > +
> > + /* default value is 0 */
> > + int filter_vlan_tagged;
> > + int filter_pppoe_tagged;
> > + int pass_vlan_indev;
> > +};
>
> I have spun on this several times, wondering if there's a way to avoid
> scratching these many bytes per netns to expose these sysctl entries
> that are plain on/off toggles... You said this:
>
> >Currently, the /proc/sys/net/bridge folder is only created in the
> >initial network namespace
>
> I think we can add one single sysctl to expose these as flags from net
> namespaces. Idea is to keep the existing (legacy) sysctl entries for
> init_net only, and add a new single new one that exposes these as flags
> (should be also available for consistency in init_net I'd suggest).
> Flags could be map in this way, eg.
>
> 0x1 call_iptables
> 0x2 call_ip6tables
> 0x4 call_arptables
> 0x8 filter_vlan_tagged
> ...
>
> Also documentation would be good to have for this.
>
> Would this idea fly for you? Thanks.
My suggestion is to keep these files per network namespace but have a
single flag argument in struct netns_brnf:
+struct netns_brnf {
+#ifdef CONFIG_SYSCTL
+struct ctl_table_header *ctl_hdr;
+#endif
+
+ /* default value is 1 */
+ unsigned int filter_flags;
+};
#define BRNF_CALL_IPTABLES0x1
#define BRNF_CALL_IP6TABLES 0x2
#define BRNF_CALL_ARPTABLES 0x4
#define BRNF_CALL_VLAN_TAGGED 0x8
a write to the corresponding file would then cause the flag to be set or
unset in filter_flags.
This way we are a) space-efficient internally not bloating struct net
while b) not breaking running tools in non-initial network namespaces
that expect the files to be there. b) is really the important bit here. :)
Christian
Re: [Bridge] [PATCH net-next 1/2] br_netfilter: add struct netns_brnf
Hi,
On Wed, Nov 07, 2018 at 02:48:58PM +0100, Christian Brauner wrote:
[...]
> diff --git a/include/net/netns/netfilter.h b/include/net/netns/netfilter.h
> index ca043342c0eb..eedbd1ac940e 100644
> --- a/include/net/netns/netfilter.h
> +++ b/include/net/netns/netfilter.h
> @@ -35,4 +35,20 @@ struct netns_nf {
> booldefrag_ipv6;
> #endif
> };
> +
> +struct netns_brnf {
> +#ifdef CONFIG_SYSCTL
> + struct ctl_table_header *ctl_hdr;
> +#endif
> +
> + /* default value is 1 */
> + int call_iptables;
> + int call_ip6tables;
> + int call_arptables;
> +
> + /* default value is 0 */
> + int filter_vlan_tagged;
> + int filter_pppoe_tagged;
> + int pass_vlan_indev;
> +};
I have spun on this several times, wondering if there's a way to avoid
scratching these many bytes per netns to expose these sysctl entries
that are plain on/off toggles... You said this:
>Currently, the /proc/sys/net/bridge folder is only created in the
>initial network namespace
I think we can add one single sysctl to expose these as flags from net
namespaces. Idea is to keep the existing (legacy) sysctl entries for
init_net only, and add a new single new one that exposes these as flags
(should be also available for consistency in init_net I'd suggest).
Flags could be map in this way, eg.
0x1 call_iptables
0x2 call_ip6tables
0x4 call_arptables
0x8 filter_vlan_tagged
...
Also documentation would be good to have for this.
Would this idea fly for you? Thanks.
Re: [Bridge] [PATCH net-next v2 1/3] net: bridge: add support for user-controlled bool options
On 26/11/2018 19:39, Stephen Hemminger wrote:
> On Sun, 25 Nov 2018 10:12:45 +0200
> Nikolay Aleksandrov wrote:
>
>> On 24/11/2018 18:46, niko...@cumulusnetworks.com wrote:
>>> On 24 November 2018 18:25:41 EET, Andrew Lunn wrote:
On Sat, Nov 24, 2018 at 06:18:33PM +0200, niko...@cumulusnetworks.com
wrote:
> On 24 November 2018 18:10:41 EET, Andrew Lunn wrote:
>>> +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id
opt,
>> bool on,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + switch (opt) {
>>> + default:
>>> + /* shouldn't be called with unsupported options */
>>> + WARN_ON(1);
>>> + break;
>>
>> So you return 0 here, meaning the br_debug() lower down will not
>> happen. Maybe return -EOPNOTSUPP?
>>
>
> No, the idea here is that some option in the future might return an
error.
> This function cannot be called with unsupported option thus the warn.
>
>>
>
> Please don't implement some part of the API until it is used (YAGNI).
> If do this kind of "someday will come" design the code will end up
> littered with dead ends.
>
Is there anything unused ? This is just a precaution to catch future
offenders which forget to handle options where they're expected.
All of the API is used.
Re: [Bridge] [PATCH net-next v2 1/3] net: bridge: add support for user-controlled bool options
On Sun, 25 Nov 2018 10:12:45 +0200
Nikolay Aleksandrov wrote:
> On 24/11/2018 18:46, niko...@cumulusnetworks.com wrote:
> > On 24 November 2018 18:25:41 EET, Andrew Lunn wrote:
> >> On Sat, Nov 24, 2018 at 06:18:33PM +0200, niko...@cumulusnetworks.com
> >> wrote:
> >>> On 24 November 2018 18:10:41 EET, Andrew Lunn wrote:
> > +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id
> >> opt,
> bool on,
> > + struct netlink_ext_ack *extack)
> > +{
> > + switch (opt) {
> > + default:
> > + /* shouldn't be called with unsupported options */
> > + WARN_ON(1);
> > + break;
>
> So you return 0 here, meaning the br_debug() lower down will not
> happen. Maybe return -EOPNOTSUPP?
>
> >>>
> >>> No, the idea here is that some option in the future might return an
> >> error.
> >>> This function cannot be called with unsupported option thus the warn.
>
Please don't implement some part of the API until it is used (YAGNI).
If do this kind of "someday will come" design the code will end up
littered with dead ends.
