Re: [PATCH v2 3/8] flow: add mac address of LAN local remote hosts to flow

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6245 bytes --]

On Thu, Jun 12, 2025 at 05:17:29PM +0200, Stefano Brivio wrote:
> On Thu, 12 Jun 2025 00:21:47 -0400
> Jon Maloy <jmaloy@redhat.com> wrote:
> 
> > When communicating with remote hosts on the local network, some guest
> > applications want to see the real mac address of that host instead
> > of passt/pasta's own tap address. The flowside structure is a convenient
> > location for storing that address, so we do that in this commit.
> > 
> > Note that we don´t add usage of this address in this commit, - that
> > will come in later commits.
> > 
> > Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> > ---
> >  flow.c | 13 ++++++++++++-
> >  flow.h |  2 ++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index da5c813..fffc817 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -20,6 +20,7 @@
> >  #include "flow.h"
> >  #include "flow_table.h"
> >  #include "repair.h"
> > +#include "netlink.h"
> >  
> >  const char *flow_state_str[] = {
> >  	[FLOW_STATE_FREE]	= "FREE",
> > @@ -438,7 +439,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  {
> >  	char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN];
> >  	struct flow_common *f = &flow->f;
> > -	const struct flowside *ini = &f->side[INISIDE];
> > +	struct flowside *ini = &f->side[INISIDE];
> >  	struct flowside *tgt = &f->side[TGTSIDE];
> >  	uint8_t tgtpif = PIF_NONE;
> >  
> > @@ -446,10 +447,16 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  	ASSERT(f->type == FLOW_TYPE_NONE);
> >  	ASSERT(f->pif[INISIDE] != PIF_NONE && f->pif[TGTSIDE] == PIF_NONE);
> >  	ASSERT(flow->f.state == FLOW_STATE_INI);
> > +	memcpy(ini->mac, c->our_tap_mac, ETH_ALEN);
> > +	memcpy(tgt->mac, c->our_tap_mac, ETH_ALEN);
> >  
> >  	switch (f->pif[INISIDE]) {
> >  	case PIF_TAP:
> >  		tgtpif = fwd_nat_from_tap(c, proto, ini, tgt);
> > +
> > +		/* If remote host on local network - insert its mac address */
> > +		if (!memcmp(&tgt->eaddr, &ini->oaddr, sizeof(ini->oaddr)))
> > +			nl_mac_get(nl_sock, &ini->oaddr, ini->mac);
> >  		break;
> >  
> >  	case PIF_SPLICE:
> > @@ -458,6 +465,10 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  
> >  	case PIF_HOST:
> >  		tgtpif = fwd_nat_from_host(c, proto, ini, tgt);
> > +
> > +		/* If remote host on local network - insert its mac address */
> > +		if (!memcmp(&tgt->oaddr, &ini->eaddr, sizeof(ini->eaddr)))
> > +			nl_mac_get(nl_sock, &tgt->oaddr, tgt->mac);
> >  		break;
> >  
> >  	default:
> > diff --git a/flow.h b/flow.h
> > index cac618a..916951b 100644
> > --- a/flow.h
> > +++ b/flow.h
> > @@ -143,12 +143,14 @@ extern const uint8_t flow_proto[];
> >   * @oaddr:	Our address (local address from passt's PoV)
> >   * @eport:	Endpoint port
> >   * @oport:	Our port
> > + * @mac:	MAC address of remote endpoint
> >   */
> >  struct flowside {
> >  	union inany_addr	oaddr;
> >  	union inany_addr	eaddr;
> >  	in_port_t		oport;
> >  	in_port_t		eport;
> > +	unsigned char 		mac[6];
> 
> We'll never have two MAC addresses that are not c->our_tap_mac in a

So... there are several things to unpack here.

1)

This series introduces cases where the statement as written above will
not be true any more. Specifically it add cases where the (tap side)
MACs are:
	- a real MAC from the host's LAN
and	- the guest's MAC

Neither of these is our_tap_mac.

1a)

I'm guessing you meant c->guest_mac, rather than c->our_tap_mac above.
In that case it's true for the time being.  It would cease to be true
if we started supporting multiple bridged guests behind a single
passt/pasta instance (in fact c->guest_mac would have to cease to
exist to allow that).

2)

If this were to go into flowside, it should be "omac" instead of just
"mac".  It's specifically "our" MAC in the sense that it's for frames
going to/from from passt itself, rather than to/from the guest
(a.k.a. the endpoint).

3)

I don't think putting this in flowside makes sense for a different
reason: putting it in flowside implies we need to track it for both
the host side and the tap side.

There are not 2, but *4* MAC addresses involved in a flow - just as
there are 4 IP addresses.  The two MACs on the host side, and the two
MACs on the tap side.

I don't think the host side omac will ever be useful.  We can't
control it, and it can change without warning due to reconfiguration
on the host.  Even without reconfiguration, multiple host interfaces
could mean the host side omac is non-constant, and non-trivial to
determine for a specific packet/flow.  In fact if there's multipath
routing, it might not even be constant for a single flow.

Host side emac is what we're determining by looking at the host ARP
table.  But all we're doing with it is using it to initialise the
tap-side omac, so I don't think we need to track it separately.  It
could also change during the life of a flow.

If the host interface is point to point (e.g. wireguard) there will be
no host side MACs at all.  If the host interface is something other
than ethernet there might be MACs, but they could have a different
format.

Tap-side omac is the key new concept in this series: we're allowing it
to be set per-flow, rather than being a global constant (our_tap_mac).

Tap-side emac remains a global constant (guest_mac) even with this
series. As above, we'd need to change that to handle multiple bridged
guests on a single passt instance.

So, to accomplish what we need for this series, we only really need to
track tap-side omac.  I agree with Stefano that it makes more sense in
flow_common than flowside.  In fact, it's even more specific than
that, since it doesn't make sense for spliced flows (since they go via
guest loopback the guest side traffic has no MACs at all).  But we
don't currently have a location for pif-specific but not
protocol-specific flow information, so flow_common is the closest we
have.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]