1. Patchew
  2. Libvirt
  3. View series

[PATCH] qemu: don't add --mac-addr option to passt commandline

Laine Stump posted 1 patch 9 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230713163255.1160501-1-laine@redhat.com
src/qemu/qemu_passt.c | 2 --
1 file changed, 2 deletions(-)
[PATCH] qemu: don't add --mac-addr option to passt commandline
Posted by Laine Stump 9 months, 3 weeks ago 
When I implemented passt support in libvirt, I saw the --mac-addr
option on the passt commandline, immediately assumed that this was
used for setting the guest interface's mac address somewhere within
passt, and read no further. As a result, "--mac-addr" is always added
to the passt commandline, specifying the setting from
<mac addr='blah'/> in the guest's interface config.

But as pointed out in this bugzilla comment:

https://bugzilla.redhat.com/2184967#c8

That is *not at all* what passt's --mac-addr option does. Instead, it
is used to force the *remote* mac address for incoming traffic to a
specific value. So setting --mac-addr results in all traffic on the
interface having the same (the guest's) mac address for both source
and destination in all traffic. Surprisingly, this still works, so
nobody noticed it during testing.

The proper thing is to not specify any mac address to passt - the
remote MAC addresses can and should remain untouched, and the local
MAC address will end up being known to passt and beyond just by the
guest sending out packets with that MAC address.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/qemu/qemu_passt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index 3679bf75fc..d36856e92e 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
     g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
     g_autoptr(virCommand) cmd = NULL;
     g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
-    char macaddr[VIR_MAC_STRING_BUFLEN];
     bool needUnlink = false;
     size_t i;
 
@@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
     virCommandAddArgList(cmd,
                          "--one-off",
                          "--socket", passtSocketName,
-                         "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
                          "--pid", pidfile,
                          NULL);
 
-- 
2.41.0
Re: [PATCH] qemu: don't add --mac-addr option to passt commandline
Posted by Stefano Brivio 9 months, 1 week ago 
On Thu, 13 Jul 2023 12:30:22 -0400
Laine Stump <laine@redhat.com> wrote:

> When I implemented passt support in libvirt, I saw the --mac-addr
> option on the passt commandline, immediately assumed that this was
> used for setting the guest interface's mac address somewhere within
> passt, and read no further. As a result, "--mac-addr" is always added
> to the passt commandline, specifying the setting from
> <mac addr='blah'/> in the guest's interface config.
> 
> But as pointed out in this bugzilla comment:
> 
> https://bugzilla.redhat.com/2184967#c8
> 
> That is *not at all* what passt's --mac-addr option does. Instead, it
> is used to force the *remote* mac address for incoming traffic to a
> specific value. So setting --mac-addr results in all traffic on the
> interface having the same (the guest's) mac address for both source
> and destination in all traffic. Surprisingly, this still works, so
> nobody noticed it during testing.
> 
> The proper thing is to not specify any mac address to passt - the
> remote MAC addresses can and should remain untouched, and the local
> MAC address will end up being known to passt and beyond just by the
> guest sending out packets with that MAC address.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 3679bf75fc..d36856e92e 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
>      g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
>      g_autoptr(virCommand) cmd = NULL;
>      g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> -    char macaddr[VIR_MAC_STRING_BUFLEN];
>      bool needUnlink = false;
>      size_t i;
>  
> @@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
>      virCommandAddArgList(cmd,
>                           "--one-off",
>                           "--socket", passtSocketName,
> -                         "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
>                           "--pid", pidfile,
>                           NULL);
>  

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano
Re: [PATCH] qemu: don't add --mac-addr option to passt commandline
Posted by Laszlo Ersek 9 months, 3 weeks ago 
On 7/13/23 18:30, Laine Stump wrote:
> When I implemented passt support in libvirt, I saw the --mac-addr
> option on the passt commandline, immediately assumed that this was
> used for setting the guest interface's mac address somewhere within
> passt, and read no further. As a result, "--mac-addr" is always added
> to the passt commandline, specifying the setting from
> <mac addr='blah'/> in the guest's interface config.
> 
> But as pointed out in this bugzilla comment:
> 
> https://bugzilla.redhat.com/2184967#c8
> 
> That is *not at all* what passt's --mac-addr option does. Instead, it
> is used to force the *remote* mac address for incoming traffic to a
> specific value. So setting --mac-addr results in all traffic on the
> interface having the same (the guest's) mac address for both source
> and destination in all traffic. Surprisingly, this still works, so
> nobody noticed it during testing.
> 
> The proper thing is to not specify any mac address to passt - the
> remote MAC addresses can and should remain untouched, and the local
> MAC address will end up being known to passt and beyond just by the
> guest sending out packets with that MAC address.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 3679bf75fc..d36856e92e 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
>      g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
>      g_autoptr(virCommand) cmd = NULL;
>      g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> -    char macaddr[VIR_MAC_STRING_BUFLEN];
>      bool needUnlink = false;
>      size_t i;
>  
> @@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
>      virCommandAddArgList(cmd,
>                           "--one-off",
>                           "--socket", passtSocketName,
> -                         "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
>                           "--pid", pidfile,
>                           NULL);
>  

I've been thinking more about this. It is certainly an improvement. Per
my reading of the passt manual, "--mac-addr" and "--gateway" should
either be specified together, or none of them should be specified; and
this change satisfies that condition.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Re: [PATCH] qemu: don't add --mac-addr option to passt commandline
Posted by Laszlo Ersek 9 months, 3 weeks ago 
On 7/13/23 18:30, Laine Stump wrote:
> When I implemented passt support in libvirt, I saw the --mac-addr
> option on the passt commandline, immediately assumed that this was
> used for setting the guest interface's mac address somewhere within
> passt, and read no further. As a result, "--mac-addr" is always added
> to the passt commandline, specifying the setting from
> <mac addr='blah'/> in the guest's interface config.
> 
> But as pointed out in this bugzilla comment:
> 
> https://bugzilla.redhat.com/2184967#c8
> 
> That is *not at all* what passt's --mac-addr option does. Instead, it
> is used to force the *remote* mac address for incoming traffic to a
> specific value. So setting --mac-addr results in all traffic on the
> interface having the same (the guest's) mac address for both source
> and destination in all traffic. Surprisingly, this still works, so
> nobody noticed it during testing.
> 
> The proper thing is to not specify any mac address to passt - the
> remote MAC addresses can and should remain untouched, and the local
> MAC address will end up being known to passt and beyond just by the
> guest sending out packets with that MAC address.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 3679bf75fc..d36856e92e 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
>      g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
>      g_autoptr(virCommand) cmd = NULL;
>      g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> -    char macaddr[VIR_MAC_STRING_BUFLEN];
>      bool needUnlink = false;
>      size_t i;
>  
> @@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
>      virCommandAddArgList(cmd,
>                           "--one-off",
>                           "--socket", passtSocketName,
> -                         "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
>                           "--pid", pidfile,
>                           NULL);
>  

I *think* we might want to keep "--mac-addr", just with the MAC address
that SLIRP assigns to the host (as seen from the guest) -- namely
"52:56:00:00:00:02".

Passt's mission is to mimic the host environment as closely as possible
in the guest env. The docs on this option includes, "Default is to use
the MAC address of the interface with the first IPv4 default route on
the host". I think that might not be entirely consistent with us setting
"--address".

I'm not sure what the best solution would be; *one* solution I can think
of is just imitating the SLIRP environment: pass all of --address,
--netmask, --gateway and --mac-addr. The first two should control the
guest's details (according to the domain XML), and the latter two should
stick with the SLIRP defaults -- the "xx.xx.2.2" host IP address, and
the 52:56:00:00:00:02 host MAC address.

Laszlo
Re: [PATCH] qemu: don't add --mac-addr option to passt commandline
Posted by Stefano Brivio 9 months, 1 week ago 
Sorry for the delay,

On Thu, 13 Jul 2023 19:27:14 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 7/13/23 18:30, Laine Stump wrote:
> > When I implemented passt support in libvirt, I saw the --mac-addr
> > option on the passt commandline, immediately assumed that this was
> > used for setting the guest interface's mac address somewhere within
> > passt, and read no further. As a result, "--mac-addr" is always added
> > to the passt commandline, specifying the setting from
> > <mac addr='blah'/> in the guest's interface config.
> > 
> > But as pointed out in this bugzilla comment:
> > 
> > https://bugzilla.redhat.com/2184967#c8
> > 
> > That is *not at all* what passt's --mac-addr option does. Instead, it
> > is used to force the *remote* mac address for incoming traffic to a
> > specific value. So setting --mac-addr results in all traffic on the
> > interface having the same (the guest's) mac address for both source
> > and destination in all traffic. Surprisingly, this still works, so
> > nobody noticed it during testing.
> > 
> > The proper thing is to not specify any mac address to passt - the
> > remote MAC addresses can and should remain untouched, and the local
> > MAC address will end up being known to passt and beyond just by the
> > guest sending out packets with that MAC address.
> > 
> > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Laine Stump <laine@redhat.com>
> > ---
> >  src/qemu/qemu_passt.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> > index 3679bf75fc..d36856e92e 100644
> > --- a/src/qemu/qemu_passt.c
> > +++ b/src/qemu/qemu_passt.c
> > @@ -176,7 +176,6 @@ qemuPasstStart(virDomainObj *vm,
> >      g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
> >      g_autoptr(virCommand) cmd = NULL;
> >      g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> > -    char macaddr[VIR_MAC_STRING_BUFLEN];
> >      bool needUnlink = false;
> >      size_t i;
> >  
> > @@ -187,7 +186,6 @@ qemuPasstStart(virDomainObj *vm,
> >      virCommandAddArgList(cmd,
> >                           "--one-off",
> >                           "--socket", passtSocketName,
> > -                         "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
> >                           "--pid", pidfile,
> >                           NULL);
> >    
> 
> I *think* we might want to keep "--mac-addr", just with the MAC address
> that SLIRP assigns to the host (as seen from the guest) -- namely
> "52:56:00:00:00:02".
> 
> Passt's mission is to mimic the host environment as closely as possible
> in the guest env. The docs on this option includes, "Default is to use
> the MAC address of the interface with the first IPv4 default route on
> the host". I think that might not be entirely consistent with us setting
> "--address".
> 
> I'm not sure what the best solution would be; *one* solution I can think
> of is just imitating the SLIRP environment: pass all of --address,
> --netmask, --gateway and --mac-addr. The first two should control the
> guest's details (according to the domain XML), and the latter two should
> stick with the SLIRP defaults -- the "xx.xx.2.2" host IP address, and
> the 52:56:00:00:00:02 host MAC address.

We had a quick discussion about this change with David, who pointed out
that, after all, we shouldn't care about the source MAC address we show
the guest being the same as what's associated to a specific interface.

As long as it's "ours", i.e. a MAC address that the host uses as
source, that should be valid and not conflicting, which is all that
matters. And the default choice fits that.

-- 
Stefano

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.