netdev - Re: [PATCH net-next 1/2] net: bridge: add support for user-controlled bool options

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181122113717.5c88bdb4@xeon-e3>
Date:   Thu, 22 Nov 2018 11:37:17 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        roopa@...ulusnetworks.com, davem@...emloft.net,
        bridge@...ts.linux-foundation.org
Subject: Re: [PATCH net-next 1/2] net: bridge: add support for
 user-controlled bool options

On Thu, 22 Nov 2018 18:01:29 +0200
Nikolay Aleksandrov <nikolay@...ulusnetworks.com> wrote:

> On 22/11/2018 17:35, Andrew Lunn wrote:
> > On Thu, Nov 22, 2018 at 06:29:24AM +0200, Nikolay Aleksandrov wrote:  
> >> We have been adding many new bridge options, a big number of which are
> >> boolean but still take up netlink attribute ids and waste space in the skb.
> >> Recently we discussed learning from link-local packets[1] and decided
> >> yet another new boolean option will be needed, thus introducing this API
> >> to save some bridge nl space.
> >> The API supports changing the value of multiple boolean options at once
> >> via the br_boolopt_multi struct which has an optmask (which options to
> >> set, bit per opt) and optval (options' new values). Future boolean
> >> options will only be added to the br_boolopt_id enum and then will have
> >> to be handled in br_boolopt_toggle/get. The API will automatically
> >> add the ability to change and export them via netlink, sysfs can use the
> >> single boolopt function versions to do the same. The behaviour with
> >> failing/succeeding is the same as with normal netlink option changing.
> >>
> >> If an option requires mapping to internal kernel flag or needs special
> >> configuration to be enabled then it should be handled in
> >> br_boolopt_toggle. It should also be able to retrieve an option's current
> >> state via br_boolopt_get.
> >>
> >> [1] https://www.spinics.net/lists/netdev/msg532698.html
> >>
> >> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> >> ---
> >>  include/uapi/linux/if_bridge.h | 18 +++++++++
> >>  include/uapi/linux/if_link.h   |  1 +
> >>  net/bridge/br.c                | 68 ++++++++++++++++++++++++++++++++++
> >>  net/bridge/br_netlink.c        | 17 ++++++++-
> >>  net/bridge/br_private.h        |  6 +++
> >>  net/core/rtnetlink.c           |  2 +-
> >>  6 files changed, 110 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> >> index e41eda3c71f1..6dc02c03bdf8 100644
> >> --- a/include/uapi/linux/if_bridge.h
> >> +++ b/include/uapi/linux/if_bridge.h
> >> @@ -292,4 +292,22 @@ struct br_mcast_stats {
> >>  	__u64 mcast_bytes[BR_MCAST_DIR_SIZE];
> >>  	__u64 mcast_packets[BR_MCAST_DIR_SIZE];
> >>  };
> >> +
> >> +/* bridge boolean options
> >> + * IMPORTANT: if adding a new option do not forget to handle
> >> + *            it in br_boolopt_toggle/get and bridge sysfs
> >> + */
> >> +enum br_boolopt_id {
> >> +	BR_BOOLOPT_MAX
> >> +};
> >> +
> >> +/* struct br_boolopt_multi - change multiple bridge boolean options
> >> + *
> >> + * @optval: new option values (bit per option)
> >> + * @optmask: options to change (bit per option)
> >> + */
> >> +struct br_boolopt_multi {
> >> +	__u32 optval;
> >> +	__u32 optmask;
> >> +};  
> > 
> > Hi Nikolay
> > 
> > Thanks for handling this.
> > 
> > How many boolean options do we already have? What it the likelihood a
> > u32 is going to be too small, in a couple of years time?
> >   
> 
> It would mean doubling the number of current options and this is only for
> boolean options so I think we're safe.
> 
> > I recently went through the pain of converting the u32 for
> > representing link modes in the phylib API to a linux bitmap.  I'm just
> > wondering if in the long run, using a linux bitmap right from the
> > beginning would be better?
> >   
> >> +int br_boolopt_multi_toggle(struct net_bridge *br,
> >> +			    struct br_boolopt_multi *bm)
> >> +{
> >> +	unsigned long bitmap = bm->optmask;
> >> +	int err = 0;
> >> +	int opt_id;
> >> +
> >> +	for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) {
> >> +		bool on = !!(bm->optval & BIT(opt_id));
> >> +
> >> +		err = br_boolopt_toggle(br, opt_id, on);
> >> +		if (err) {
> >> +			br_debug(br, "boolopt multi-toggle error: option: %d current: %d new: %d error: %d\n",
> >> +				 opt_id, br_boolopt_get(br, opt_id), on, err);  
> > 
> > Would it be possible to return that to userspace using the extended
> > error infrastructure?
> >   
> 
> No, it doesn't support dynamic messages AFAIK.
> 
> >       Andrew
> >   
> 

My concern is about backwards compatibility. What about old userspace and new userspace tools with old kernels.
Having multiple bits does allow handling cases where certain combos won't work.