netdev - Re: [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to add (*, G) with a source list and filter mode

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] [day] [month] [year] [list]

Date:   Thu, 3 Nov 2022 11:09:30 +0200
From:   Ido Schimmel <idosch@...dia.com>
To:     Nikolay Aleksandrov <razor@...ckwall.org>
Cc:     netdev@...r.kernel.org, bridge@...ts.linux-foundation.org,
        davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, roopa@...dia.com, mlxsw@...dia.com
Subject: Re: [RFC PATCH net-next 17/19] bridge: mcast: Allow user space to
 add (*, G) with a source list and filter mode

On Wed, Oct 19, 2022 at 04:28:23PM +0300, Nikolay Aleksandrov wrote:
> On 18/10/2022 15:04, Ido Schimmel wrote:
> > +static int br_mdb_config_src_list_init(struct nlattr *src_list,
> > +				       struct br_mdb_config *cfg,
> > +				       struct netlink_ext_ack *extack)
> > +{
> > +	struct br_mdb_src_entry *src, *tmp;
> > +	struct nlattr *src_entry;
> > +	int rem, err;
> > +
> > +	nla_for_each_nested(src_entry, src_list, rem) {
> > +		err = br_mdb_config_src_entry_init(src_entry, cfg, extack);
> 
> Hmm, since we know the exact number of these (due to attr embedding) can't we allocate
> all at once and drop the list? They should not be more than 32 (PG_SRC_ENT_LIMIT) IIRC,
> which makes it at most 1152 bytes. Might simplify the code a bit and reduce allocations.

I didn't see how I can reliably determine the exact number of source
entries without going all the 'MDBE_SRC_LIST_ENTRY' attributes. I mean,
the entries can have varying sizes in case user space provided mixed
IPv4/IPv6 sources (which will be rejected later on) and in the future we
might have more attributes per-source entry other than the address
(e.g., source timer), which might be specified only for a subset of the
entries.

So, I did end up converting to an array like you suggested, but I'm
going over the entries twice. Once to understand how large the array
should be and again to initialize it. It's control path so it should be
fine. The advantages are that the number of allocations are reduced and
that I can reject a too long source list before doing any processing:

if (cfg->num_src_entries >= PG_SRC_ENT_LIMIT) {
	NL_SET_ERR_MSG_FMT_MOD(extack, "Exceeded maximum number of source entries (%u)",
			       PG_SRC_ENT_LIMIT);
	return -EINVAL;
}

[...]

> > +static void br_mdb_config_fini(struct br_mdb_config *cfg)
> > +{
> > +	br_mdb_config_attrs_fini(cfg);
> > +}
> > +
> 
> Is there more coming to these two _fini helpers? If not, I think one would be enough, i.e.
> just call br_mdb_config_src_list_fini() from br_mdb_config_fini()

Done.

Thanks!