netdev - Re: [RFC PATCH 2/3] net: expose ebridge FDB with priv flag IFF_OFFLOADED

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: <1330718193.2611.36.camel@bwh-desktop>
Date:	Fri, 2 Mar 2012 19:56:33 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	John Fastabend <john.r.fastabend@...el.com>
CC:	<jhs@...atatu.com>, <shemminger@...tta.com>,
	<kernel@...tstofly.org>, <hadi@...erus.ca>, <roprabhu@...co.com>,
	<mst@...hat.com>, <netdev@...r.kernel.org>,
	<gregory.v.rose@...el.com>, <davem@...emloft.net>
Subject: Re: [RFC PATCH 2/3] net: expose ebridge FDB with priv flag
 IFF_OFFLOADED_FDB

On Tue, 2012-02-28 at 23:17 -0800, John Fastabend wrote:
> This adds a new private interface flag IFF_OFFLOADED_FDB and an
> additional ndmsg flag NTF_EMBEDDED.
> 
> The private flag IFF_OFFLOADED_FDB should be set on devices to
> indicate an embedded bridging component exists with a forwarding
> database.
> 
> With this set PF_BRIDGE:{RTM_NEWNEIGH|RTM_DELNEIGH|RTM_GETNEIGH}
> netlink msgs can manage the unicast address list on these devices
> by setting the NTF_EMBEDDED flag in ndm_flags.
> 
> These commands are compatible with the SW bridge allowing the same
> user space tools to be used with both SW bridges and HW bridges.
[...]
> index 9e70191..7b1a581 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -211,6 +211,57 @@ static int br_validate(struct nlattr *tb[], struct nlattr *data[])
>  	return 0;
>  }
>  
> +static int rtnl_offloaded_fdb_add(struct nlmsghdr *nlh, struct net_device *dev)
> +{
> +	struct ndmsg *ndm;
> +	struct netdev_hw_addr *ha;
> +	struct nlattr *tb[NDA_MAX+1];
> +	__u8 *addr;
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	if (!(dev->priv_flags & IFF_OFFLOADED_FDB))
> +		return -ENODEV;

EOPNOTSUPP

> +	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	ndm = nlmsg_data(nlh);
> +	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> +		pr_info("fdb_add: RTM_NEWNEIGH with invalid address\n");
> +		return -EINVAL;
> +	}
> +
> +	addr = nla_data(tb[NDA_LLADDR]);
> +	if (!is_valid_ether_addr(addr)) {
> +		pr_info("fdb_add: RTM_NEWNEIGH with invalid ether address\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ndm->ndm_state & NUD_PERMANENT) {
> +		pr_info("fdb_add: RTM_NEWNEIGH with invalid state %#x\n",
> +			ndm->ndm_state);
> +		return -EINVAL;
> +	}
> +
> +	if (is_multicast_ether_addr(addr))
> +		return -EINVAL;

There is a pending change to the ndo_validate_addr interface which I
think would allow us to replace the Ethernet-specific checks with a
check against dev->addr_len and a call to ndo_validate_addr.  Not that I
really care about anything other than Ethernet, but it would be a little
more elegant.

> +	netif_addr_lock_bh(dev);
> +	list_for_each_entry(ha, &dev->uc.list, list) {
> +		if (!compare_ether_addr(ha->addr, addr)) {
> +			netif_addr_unlock_bh(dev);
> +			return -EEXIST;
> +		}
> +	}
> +	netif_addr_unlock_bh(dev);
> +
> +	err = dev_uc_add(dev, addr);
[...]

The software bridge makes the duplicate check conditional on NLM_F_EXCL.
Shouldn't this be consistent?

Also, this seems racy.  If all manipulation of the UC address list is
serialised by RTNL (which I think in practice it is) then there is no
race and we also don't need to take the netif_addr_lock(), but if we're
not allowed to assume that then we shouldn't drop the lock.  Perhaps the
check can be moved into a dev_uc_add_exclusive() or something like that.

General comment: why should the software bridge code be loaded in order
to control a hardware bridge?  (It's not even the only software bridge
we have!)  It would perhaps be neater to have separate modules for the
software bridge, the rtnetlink bridge ops, and the glue functions for
hardware bridges.  But that could be done later.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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