All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jay Vosburgh <jay.vosburgh@canonical.com>
To: Jarod Wilson <jarod@redhat.com>
Cc: linux-kernel@vger.kernel.org,
	Veaceslav Falico <vfalico@gmail.com>,
	Andy Gospodarek <andy@greyhouse.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Thomas Davis <tadavis@lbl.gov>,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next v2 2/4] bonding/balance-lb: don't rewrite bridged non-local MACs
Date: Fri, 21 May 2021 10:58:57 -0700	[thread overview]
Message-ID: <21329.1621619937@famine> (raw)
In-Reply-To: <20210521132756.1811620-3-jarod@redhat.com>

Jarod Wilson <jarod@redhat.com> wrote:

>With a virtual machine behind a bridge that directly incorporates a
>balance-alb bond as one of its ports, outgoing traffic should retain the
>VM's source MAC. That works fine most of the time, until doing a failover,
>and then the MAC gets rewritten to the bond slave's MAC, and the return
>traffic gets dropped. If we don't rewrite the MAC there, we don't lose the
>traffic.

	Comparing the description above to the patch, is the erroneous
behavior really related to failover (i.e., bond slave goes down, bond
reshuffles various things as it is wont to do), or is it related to
either a TX side rebalance or even simply that specific traffic is being
sent on a slave that isn't the curr_active_slave?

	One more comment, below.

>Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>Cc: Veaceslav Falico <vfalico@gmail.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Cc: "David S. Miller" <davem@davemloft.net>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Cc: Thomas Davis <tadavis@lbl.gov>
>Cc: netdev@vger.kernel.org
>Signed-off-by: Jarod Wilson <jarod@redhat.com>
>---
> drivers/net/bonding/bond_alb.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index 3455f2cc13f2..c57f62e43328 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -1302,6 +1302,23 @@ void bond_alb_deinitialize(struct bonding *bond)
> 		rlb_deinitialize(bond);
> }
> 
>+static bool bond_alb_bridged_mac(struct bonding *bond, struct ethhdr *eth_data)
>+{
>+	if (BOND_MODE(bond) != BOND_MODE_ALB)
>+		return false;
>+
>+	/* Don't modify source MACs that do not originate locally
>+	 * (e.g.,arrive via a bridge).
>+	 */
>+	if (!netif_is_bridge_port(bond->dev))
>+		return false;

	Repeating my comment (from my response to the v1 patch) that
hasn't been addressed:

	I believe this logic will fail if the plumbing is, e.g., bond ->
vlan -> bridge, as netif_is_bridge_port() would not return true for the
bond in that case.

	Making this reliable is tricky at best, and may be impossible to
be correct for all possible cases.  As such, I think the comment above
should reflect the limited scope of what is actually being checked here
(i.e., the bond itself is directly a bridge port).

	Ideally, the bonding.rst documentation should describe the
special behaviors of alb mode when configured as a bridge port.

	-J

>+
>+	if (bond_slave_has_mac_rx(bond, eth_data->h_source))
>+		return false;
>+
>+	return true;
>+}
>+
> static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> 				    struct slave *tx_slave)
> {
>@@ -1316,7 +1333,8 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> 	}
> 
> 	if (tx_slave && bond_slave_can_tx(tx_slave)) {
>-		if (tx_slave != rcu_access_pointer(bond->curr_active_slave)) {
>+		if (tx_slave != rcu_access_pointer(bond->curr_active_slave) &&
>+		    !bond_alb_bridged_mac(bond, eth_data)) {
> 			ether_addr_copy(eth_data->h_source,
> 					tx_slave->dev->dev_addr);
> 		}
>-- 
>2.30.2

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

  reply	other threads:[~2021-05-21 17:59 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18 21:08 [PATCH 0/4] bond_alb: support VMs behind bridges better Jarod Wilson
2021-05-18 21:08 ` [PATCH 1/4] bonding: add pure source-mac-based tx hashing option Jarod Wilson
2021-05-19  9:01   ` Nikolay Aleksandrov
2021-05-18 21:08 ` [PATCH 2/4] bond_alb: don't rewrite bridged non-local MACs Jarod Wilson
2021-05-19 22:31   ` Jay Vosburgh
2021-05-18 21:08 ` [PATCH 3/4] bond_alb: don't tx balance multicast traffic either Jarod Wilson
2021-05-19 18:45   ` Jay Vosburgh
2021-05-18 21:08 ` [PATCH 4/4] bond_alb: put all slaves into promisc Jarod Wilson
2021-05-19 16:47   ` Jay Vosburgh
2021-05-21 13:27 ` [PATCH net-next v2 0/4] bonding/balance-alb: support VMs behind bridges better Jarod Wilson
2021-05-21 13:27   ` [PATCH net-next v2 1/4] bonding: add pure source-mac-based tx hashing option Jarod Wilson
2021-05-21 13:39     ` Nikolay Aleksandrov
2021-05-21 13:41       ` Nikolay Aleksandrov
2021-05-21 18:01       ` Jay Vosburgh
2021-05-21 13:27   ` [PATCH net-next v2 2/4] bonding/balance-lb: don't rewrite bridged non-local MACs Jarod Wilson
2021-05-21 17:58     ` Jay Vosburgh [this message]
2021-05-21 13:27   ` [PATCH net-next v2 3/4] bonding/balance-alb: don't tx balance multicast traffic either Jarod Wilson
2021-05-21 17:02     ` Jay Vosburgh
2021-05-21 13:27   ` [PATCH net-next v2 4/4] bonding/balance-alb: put all slaves into promisc Jarod Wilson
2021-05-21 17:01     ` Jay Vosburgh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=21329.1621619937@famine \
    --to=jay.vosburgh@canonical.com \
    --cc=andy@greyhouse.net \
    --cc=davem@davemloft.net \
    --cc=jarod@redhat.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=tadavis@lbl.gov \
    --cc=vfalico@gmail.com \
    --subject='Re: [PATCH net-next v2 2/4] bonding/balance-lb: don'\''t rewrite bridged non-local MACs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.