From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Subject: Re: [PATCH net] bonding: Fix stacked device detection in arp monitoring Date: Wed, 07 May 2014 09:20:04 -0400 Message-ID: <536A3304.5020300@redhat.com> References: <1399398075-16323-1-git-send-email-vyasevic@redhat.com> <21361.1399404141@localhost.localdomain> Reply-To: vyasevic@redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Veaceslav Falico , Andy Gospodarek , Ding Tianhong , Patric McHardy To: Jay Vosburgh Return-path: Received: from mx1.redhat.com ([209.132.183.28]:10640 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932821AbaEGOS7 (ORCPT ); Wed, 7 May 2014 10:18:59 -0400 In-Reply-To: <21361.1399404141@localhost.localdomain> Sender: netdev-owner@vger.kernel.org List-ID: On 05/06/2014 03:22 PM, Jay Vosburgh wrote: > Vlad Yasevich wrote: > >> Prior to commit fbd929f2dce460456807a51e18d623db3db9f077 >> bonding: support QinQ for bond arp interval >> >> the arp monitoring code allowed for proper detection of devices >> stacked on top of vlans. Since the above commit, the >> code can still detect a device stacked on top of single >> vlan, but not a device stacked on top of Q-in-Q configuration. >> The search will only set the inner vlan tag if the route >> device is the vlan device. However, this is not always the >> case, as it is possible to extend the stacked configuration. >> >> With this patch it is possible to provision devices on >> top Q-in-Q vlan configuration that should be used as >> a source of ARP monitoring information. >> >> For example: >> ip link add link bond0 vlan10 type vlan proto 802.1q id 10 >> ip link add link vlan10 vlan100 type vlan proto 802.1q id 100 >> ip link add link vlan100 type macvlan >> >> Note: This patch limits the number of stacked VLANs to 2, >> just like before. The original, however had another issue >> in that if we had more then 2 levels of VLANs, we would end >> up generating incorrectly tagged traffic. This is no longer >> possible. > > I like this in general, with one minor exception: we lose the > "inner" and "outer" nomenclature, which I think made the code clearer. I can completely remove the inner/outer nomenclature if you wish, or can save it for follow-on patch. > I think the bond_collect_vlans function deserves at least a sentence or > two of commentary explaining that we're walking up the layers from > bottom to top, and collecting the outer tag and then the inner tag, > thus, outer is tags[0] and inner is tags[1]. Will do. > > And, just as a random observation, it looks like it will do the > right thing (or at least not crash) if things are layered in odd ways, > e.g., if the macvlan is nested between the vlans. Right. There is a slight bug though that I found last night. I'll fix that in v2 as well. Thanks -vlad > > -J > >> Fixes: fbd929f2dce460456807a51e18d623db3db9f077 (bonding: support QinQ for bond arp interval) >> CC: Jay Vosburgh >> CC: Veaceslav Falico >> CC: Andy Gospodarek >> CC: Ding Tianhong >> CC: Patric McHardy >> Signed-off-by: Vlad Yasevich >> --- >> drivers/net/bonding/bond_main.c | 90 +++++++++++++++++------------------------ >> 1 file changed, 37 insertions(+), 53 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 9d08e00..4822728 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -2164,22 +2164,46 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, >> arp_xmit(skb); >> } >> >> +static bool bond_collect_vlans(struct net_device *start, struct net_device *end, >> + struct bond_vlan_tag *tag, int idx) >> +{ >> + struct net_device *upper; >> + struct list_head *iter; >> + >> + /* We do not support more then 2 levels of VLAN nesting */ >> + if (idx >= 2) >> + return false; >> + >> + netdev_for_each_all_upper_dev_rcu(start, upper, iter) { >> + if (is_vlan_dev(upper)) { >> + tag[idx].vlan_proto = vlan_dev_vlan_proto(upper); >> + tag[idx].vlan_id = vlan_dev_vlan_id(upper); >> + idx++; >> + } >> + if (upper == end) >> + return true; >> + >> + /* Look at upper devices list only if current device >> + * is a vlan >> + */ >> + if (is_vlan_dev(upper) && bond_collect_vlans(upper, end, tag, idx)) >> + return true; >> + } >> + return false; >> +} >> + >> >> static void bond_arp_send_all(struct bonding *bond, struct slave *slave) >> { >> - struct net_device *upper, *vlan_upper; >> - struct list_head *iter, *vlan_iter; >> struct rtable *rt; >> - struct bond_vlan_tag inner, outer; >> + struct bond_vlan_tag tags[2]; >> __be32 *targets = bond->params.arp_targets, addr; >> int i; >> + bool ret; >> >> for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) { >> pr_debug("basa: target %pI4\n", &targets[i]); >> - inner.vlan_proto = 0; >> - inner.vlan_id = 0; >> - outer.vlan_proto = 0; >> - outer.vlan_id = 0; >> + memset(tags, 0, sizeof(tags)); >> >> /* Find out through which dev should the packet go */ >> rt = ip_route_output(dev_net(bond->dev), targets[i], 0, >> @@ -2192,7 +2216,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) >> net_warn_ratelimited("%s: no route to arp_ip_target %pI4 and arp_validate is set\n", >> bond->dev->name, >> &targets[i]); >> - bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], 0, &inner, &outer); >> + bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], 0, &tags[1], &tags[0]); >> continue; >> } >> >> @@ -2201,52 +2225,12 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) >> goto found; >> >> rcu_read_lock(); >> - /* first we search only for vlan devices. for every vlan >> - * found we verify its upper dev list, searching for the >> - * rt->dst.dev. If found we save the tag of the vlan and >> - * proceed to send the packet. >> - */ >> - netdev_for_each_all_upper_dev_rcu(bond->dev, vlan_upper, >> - vlan_iter) { >> - if (!is_vlan_dev(vlan_upper)) >> - continue; >> - >> - if (vlan_upper == rt->dst.dev) { >> - outer.vlan_proto = vlan_dev_vlan_proto(vlan_upper); >> - outer.vlan_id = vlan_dev_vlan_id(vlan_upper); >> - rcu_read_unlock(); >> - goto found; >> - } >> - netdev_for_each_all_upper_dev_rcu(vlan_upper, upper, >> - iter) { >> - if (upper == rt->dst.dev) { >> - /* If the upper dev is a vlan dev too, >> - * set the vlan tag to inner tag. >> - */ >> - if (is_vlan_dev(upper)) { >> - inner.vlan_proto = vlan_dev_vlan_proto(upper); >> - inner.vlan_id = vlan_dev_vlan_id(upper); >> - } >> - outer.vlan_proto = vlan_dev_vlan_proto(vlan_upper); >> - outer.vlan_id = vlan_dev_vlan_id(vlan_upper); >> - rcu_read_unlock(); >> - goto found; >> - } >> - } >> - } >> - >> - /* if the device we're looking for is not on top of any of >> - * our upper vlans, then just search for any dev that >> - * matches, and in case it's a vlan - save the id >> - */ >> - netdev_for_each_all_upper_dev_rcu(bond->dev, upper, iter) { >> - if (upper == rt->dst.dev) { >> - rcu_read_unlock(); >> - goto found; >> - } >> - } >> + ret = bond_collect_vlans(bond->dev, rt->dst.dev, tags, 0); >> rcu_read_unlock(); >> >> + if (ret) >> + goto found; >> + >> /* Not our device - skip */ >> pr_debug("%s: no path to arp_ip_target %pI4 via rt.dev %s\n", >> bond->dev->name, &targets[i], >> @@ -2259,7 +2243,7 @@ found: >> addr = bond_confirm_addr(rt->dst.dev, targets[i], 0); >> ip_rt_put(rt); >> bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], >> - addr, &inner, &outer); >> + addr, &tags[1], &tags[0]); >> } >> } > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com >