Re: [net-next PATCH v2 2/3] net: set and query VEB/VEPA bridge mode via PF_BRIDGE

On 11/2/2012 3:38 PM, Ben Hutchings wrote:

On Wed, 2012-10-24 at 11:13 -0700, John Fastabend wrote:

quoted

Hardware switches may support enabling and disabling the
loopback switch which puts the device in a VEPA mode defined
in the IEEE 802.1Qbg specification. In this mode frames are
not switched in the hardware but sent directly to the switch.
SR-IOV capable NICs will likely support this mode I am
aware of at least two such devices. Also I am told (but don't
have any of this hardware available) that there are devices
that only support VEPA modes. In these cases it is important
at a minimum to be able to query these attributes.

This patch adds an additional IFLA_BRIDGE_MODE attribute that can be
set and dumped via the PF_BRIDGE:{SET|GET}LINK operations. Also
anticipating bridge attributes that may be common for both embedded
bridges and software bridges this adds a flags attribute
IFLA_BRIDGE_FLAGS currently used to determine if the command or event
is being generated to/from an embedded bridge or software bridge.
Finally, the event generation is pulled out of the bridge module and
into rtnetlink proper.

[...]

quoted

--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

[...]

quoted

+static int rtnl_bridge_notify(struct net_device *dev, u16 flags)
+{
+	struct net *net = dev_net(dev);
+	struct net_device *master = dev->master;
+	struct sk_buff *skb;
+	int err = -EOPNOTSUPP;
+
+	skb = nlmsg_new(bridge_nlmsg_size(), GFP_ATOMIC);
+	if (!skb) {
+		err = -ENOMEM;
+		goto errout;
+	}
+
+	if (!flags && master && master->netdev_ops->ndo_bridge_getlink)
+		err = master->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev);
+	else if (dev->netdev_ops->ndo_bridge_getlink)
+		err = dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev);

This doesn't make sense.  If flags == BRIDGE_FLAGS_MASTER then we only
send the 'self' information.  If flags == BRIDGE_FLAGS_MASTER |
BRIDGE_FLAGS_SELF then we only send the master information.

agh. I screwed this up when I made this handle setting both master
and self flags. I'll fix it. Thanks.

[...]

quoted

  static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
  			       void *arg)
  {
  	struct net *net = sock_net(skb->sk);
  	struct ifinfomsg *ifm;
  	struct net_device *dev;
-	int err = -EINVAL;
+	struct nlattr *br_spec, *attr = NULL;
+	int rem, err = -EOPNOTSUPP;
+	u16 flags = 0;

  	if (nlmsg_len(nlh) < sizeof(*ifm))
  		return -EINVAL;

@@ -2316,15 +2363,45 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
  		return -ENODEV;
  	}

-	if (dev->master && dev->master->netdev_ops->ndo_bridge_setlink) {
+	br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
+	if (br_spec) {
+		nla_for_each_nested(attr, br_spec, rem) {
+			if (nla_type(attr) == IFLA_BRIDGE_FLAGS) {
+				flags = nla_get_u16(attr);
+				break;
+			}
+		}
+	}
+
+	if (!flags || (flags & BRIDGE_FLAGS_MASTER)) {
+		if (!dev->master ||
+		    !dev->master->netdev_ops->ndo_bridge_setlink) {
+			err = -EOPNOTSUPP;
+			goto out;
+		}
+
  		err = dev->master->netdev_ops->ndo_bridge_setlink(dev, nlh);
  		if (err)
  			goto out;
+
+		flags &= ~BRIDGE_FLAGS_MASTER;
  	}

-	if (dev->netdev_ops->ndo_bridge_setlink)
-		err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
+	if ((flags & BRIDGE_FLAGS_SELF)) {
+		if (!dev->netdev_ops->ndo_bridge_setlink)
+			err = -EOPNOTSUPP;
+		else
+			err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
+
+		if (!err)
+			flags &= ~BRIDGE_FLAGS_SELF;
+	}

+	if (attr && nla_type(attr) == IFLA_BRIDGE_FLAGS)

This condition is wrong; attr will *not* be NULL if the
nla_for_each_nested() loop terminates without finding an
IFLA_BRIDGE_FLAGS attribute.

It might be NULL if the nlmsg has no IFLA_AF_SPEC attr. In this case
we still need to send the PROTINFO attribute to the master which
could be the linux bridge.

quoted

+		memcpy(nla_data(attr), &flags, sizeof(flags));
+	/* Generate event to notify upper layer of bridge change */
+	if (!err)
+		err = rtnl_bridge_notify(dev, flags);

flags has been modified above; surely we want to use the original value
here?

Yes. Thanks Ben. I'll have a patch shortly.

Ben.

quoted

  out:
  	return err;
  }