netdev - Re: [PATCH net-next v1 2/2] bridge: export multicast database via netlink

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]

Date:	Fri, 30 Nov 2012 23:00:52 +0800
From:	Cong Wang <amwang@...hat.com>
To:	Thomas Graf <tgraf@...g.ch>
Cc:	netdev@...r.kernel.org, bridge@...ts.linux-foundation.org,
	Herbert Xu <herbert@...dor.hengli.com.au>,
	Stephen Hemminger <shemminger@...tta.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH net-next v1 2/2] bridge: export multicast database via
 netlink

On Fri, 2012-11-30 at 11:26 +0000, Thomas Graf wrote:
> On 11/30/12 at 05:58pm, Cong Wang wrote:
> > +
> > +	nest = nla_nest_start(skb, MDBA_ROUTER);
> > +	if (nest == NULL)
> > +		return -EMSGSIZE;
> > +
> > +	hlist_for_each_entry_rcu(p, n, &br->router_list, rlist) {
> > +		if (p && nla_put_u16(skb, MDBA_BRPORT_NO, p->port_no))
> > +			goto fail;
> > +	}
> 
> port_no 0 is reserved, right?
> 
> We can reduce message size here and make it easier to extend by
> using p->port_no as the attribute id by doing something like this:
> 
> hlist_for_each_entry_rcu(p, n, &br->router_list, rlist)
> 	if (nla_put_flag(skb, p->port_no))
> 		goto fail;
> 
> This will result in an empty attribute body for now and if you ever
> need to include more data you can simply start putting attributes
> insde that empty body and old users will continue to function.


I don't understand this. nla_put_flag() is used to put a flag (only one
bit set) into a netlink message, so why should we use it to put
p->port_no here? And why port_no 0 matters here?

[...]

> > +
> > +	cb->seq = mdb->seq;
> 
> I'm not sure how this is supposed to worl. cb->seq may not change
> throughout the complete dump process or the dump will be interrupted
> and the user is required to restart.
> 
> Each bridge will have its own mdb resulting in a differing seq.
> 

So I should use net->dev_base_seq + mdb->seq ?


All of the rest suggestions are taken by me.

Thanks for your detailed review!

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html