tap-linux: Open ipvtap and macvtap

[PATCH] tap-linux: Open ipvtap and macvtap

Posted by Akihiko Odaki 1 month, 2 weeks ago

ipvtap and macvtap create a file for each interface unlike tuntap, which
creates one file shared by all interfaces. Try to open a file dedicated
to the interface first for ipvtap and macvtap.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 net/tap-linux.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/tap-linux.c b/net/tap-linux.c
index 1226d5fda2d9..22ec2f45d2b7 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -45,10 +45,21 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     int len = sizeof(struct virtio_net_hdr);
     unsigned int features;
 
-    fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
+
+    ret = if_nametoindex(ifname);
+    if (ret) {
+        g_autofree char *file = g_strdup_printf("/dev/tap%d", ret);
+        fd = open(file, O_RDWR);
+    } else {
+        fd = -1;
+    }
+
     if (fd < 0) {
-        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
-        return -1;
+        fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
+        if (fd < 0) {
+            error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
+            return -1;
+        }
     }
     memset(&ifr, 0, sizeof(ifr));
     ifr.ifr_flags = IFF_TAP | IFF_NO_PI;

---
base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
change-id: 20241008-macvtap-b152e5abb457

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH] tap-linux: Open ipvtap and macvtap

Posted by Jason Wang 1 month, 2 weeks ago

On Tue, Oct 8, 2024 at 2:52 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> ipvtap and macvtap create a file for each interface unlike tuntap, which
> creates one file shared by all interfaces. Try to open a file dedicated
> to the interface first for ipvtap and macvtap.
>

Management layers usually pass these fds via SCM_RIGHTS. Is this for
testing purposes? (Note that we can use something like -netdev
tap,fd=10 10<>/dev/tap0).

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  net/tap-linux.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 1226d5fda2d9..22ec2f45d2b7 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -45,10 +45,21 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>      int len = sizeof(struct virtio_net_hdr);
>      unsigned int features;
>
> -    fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
> +
> +    ret = if_nametoindex(ifname);
> +    if (ret) {
> +        g_autofree char *file = g_strdup_printf("/dev/tap%d", ret);
> +        fd = open(file, O_RDWR);
> +    } else {
> +        fd = -1;
> +    }
> +
>      if (fd < 0) {
> -        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
> -        return -1;
> +        fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));

Any reason tuntap were tried after the macvtap/ipvtap?

> +        if (fd < 0) {
> +            error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
> +            return -1;
> +        }
>      }
>      memset(&ifr, 0, sizeof(ifr));
>      ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
>
> ---
> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
> change-id: 20241008-macvtap-b152e5abb457
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>

Thanks

>

Re: [PATCH] tap-linux: Open ipvtap and macvtap

Posted by Akihiko Odaki 1 month, 1 week ago

On 2024/10/09 16:41, Jason Wang wrote:
> On Tue, Oct 8, 2024 at 2:52 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> ipvtap and macvtap create a file for each interface unlike tuntap, which
>> creates one file shared by all interfaces. Try to open a file dedicated
>> to the interface first for ipvtap and macvtap.
>>
> 
> Management layers usually pass these fds via SCM_RIGHTS. Is this for
> testing purposes? (Note that we can use something like -netdev
> tap,fd=10 10<>/dev/tap0).

I used this for testing.

> 
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   net/tap-linux.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/tap-linux.c b/net/tap-linux.c
>> index 1226d5fda2d9..22ec2f45d2b7 100644
>> --- a/net/tap-linux.c
>> +++ b/net/tap-linux.c
>> @@ -45,10 +45,21 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>>       int len = sizeof(struct virtio_net_hdr);
>>       unsigned int features;
>>
>> -    fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
>> +
>> +    ret = if_nametoindex(ifname);
>> +    if (ret) {
>> +        g_autofree char *file = g_strdup_printf("/dev/tap%d", ret);
>> +        fd = open(file, O_RDWR);
>> +    } else {
>> +        fd = -1;
>> +    }
>> +
>>       if (fd < 0) {
>> -        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
>> -        return -1;
>> +        fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
> 
> Any reason tuntap were tried after the macvtap/ipvtap?

If we try tuntap first, we will know that it is not tuntap when calling 
TUNSETIFF. We will need to call TUNGETFEATURES and TUNSETVNETHDRSZ again 
in such a case because they precede TUNSETIFF. Calling them twice is 
troublesome.

This is also consistent with libvirt. libvirt first checks if 
g_strdup_printf("/dev/tap%d", ifindex) exists, and falls back to tuntap 
otherwise.

Regards,
Akihiko Odaki

> 
>> +        if (fd < 0) {
>> +            error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
>> +            return -1;
>> +        }
>>       }
>>       memset(&ifr, 0, sizeof(ifr));
>>       ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
>>
>> ---
>> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
>> change-id: 20241008-macvtap-b152e5abb457
>>
>> Best regards,
>> --
>> Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Thanks
> 
>>
>

Re: [PATCH] tap-linux: Open ipvtap and macvtap

Posted by Jason Wang 1 month ago

On Sat, Oct 12, 2024 at 5:05 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/09 16:41, Jason Wang wrote:
> > On Tue, Oct 8, 2024 at 2:52 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> ipvtap and macvtap create a file for each interface unlike tuntap, which
> >> creates one file shared by all interfaces. Try to open a file dedicated
> >> to the interface first for ipvtap and macvtap.
> >>
> >
> > Management layers usually pass these fds via SCM_RIGHTS. Is this for
> > testing purposes? (Note that we can use something like -netdev
> > tap,fd=10 10<>/dev/tap0).
>
> I used this for testing.

Anything that prevents you from using fd redirection? If not
management interest and we had already had a way for testing, I tend
to not introduce new code as it may bring bugs.

>
> >
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   net/tap-linux.c | 17 ++++++++++++++---
> >>   1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/tap-linux.c b/net/tap-linux.c
> >> index 1226d5fda2d9..22ec2f45d2b7 100644
> >> --- a/net/tap-linux.c
> >> +++ b/net/tap-linux.c
> >> @@ -45,10 +45,21 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
> >>       int len = sizeof(struct virtio_net_hdr);
> >>       unsigned int features;
> >>
> >> -    fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
> >> +
> >> +    ret = if_nametoindex(ifname);
> >> +    if (ret) {
> >> +        g_autofree char *file = g_strdup_printf("/dev/tap%d", ret);
> >> +        fd = open(file, O_RDWR);
> >> +    } else {
> >> +        fd = -1;
> >> +    }
> >> +
> >>       if (fd < 0) {
> >> -        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
> >> -        return -1;
> >> +        fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
> >
> > Any reason tuntap were tried after the macvtap/ipvtap?
>
> If we try tuntap first, we will know that it is not tuntap when calling
> TUNSETIFF. We will need to call TUNGETFEATURES and TUNSETVNETHDRSZ again
> in such a case because they precede TUNSETIFF. Calling them twice is
> troublesome.

I may miss something, we are only at the phase of open() not TUNSETIFF?

>
> This is also consistent with libvirt. libvirt first checks if
> g_strdup_printf("/dev/tap%d", ifindex) exists, and falls back to tuntap
> otherwise.

This is not what I understand from how layered products work. Libvirt
should align with Qemu for low level things like TAP, not the reverse.

Thanks

>
> Regards,
> Akihiko Odaki
>
> >
> >> +        if (fd < 0) {
> >> +            error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
> >> +            return -1;
> >> +        }
> >>       }
> >>       memset(&ifr, 0, sizeof(ifr));
> >>       ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
> >>
> >> ---
> >> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
> >> change-id: 20241008-macvtap-b152e5abb457
> >>
> >> Best regards,
> >> --
> >> Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > Thanks
> >
> >>
> >
>

Re: [PATCH] tap-linux: Open ipvtap and macvtap

Posted by Akihiko Odaki 1 month ago

On 2024/10/18 17:10, Jason Wang wrote:
> On Sat, Oct 12, 2024 at 5:05 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/10/09 16:41, Jason Wang wrote:
>>> On Tue, Oct 8, 2024 at 2:52 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> ipvtap and macvtap create a file for each interface unlike tuntap, which
>>>> creates one file shared by all interfaces. Try to open a file dedicated
>>>> to the interface first for ipvtap and macvtap.
>>>>
>>>
>>> Management layers usually pass these fds via SCM_RIGHTS. Is this for
>>> testing purposes? (Note that we can use something like -netdev
>>> tap,fd=10 10<>/dev/tap0).
>>
>> I used this for testing.
> 
> Anything that prevents you from using fd redirection? If not
> management interest and we had already had a way for testing, I tend
> to not introduce new code as it may bring bugs.

I don't know what ifindex the macvtap device has so it's easier to use 
if QEMU can automatically figure out the it.

> 
>>
>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    net/tap-linux.c | 17 ++++++++++++++---
>>>>    1 file changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/tap-linux.c b/net/tap-linux.c
>>>> index 1226d5fda2d9..22ec2f45d2b7 100644
>>>> --- a/net/tap-linux.c
>>>> +++ b/net/tap-linux.c
>>>> @@ -45,10 +45,21 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>>>>        int len = sizeof(struct virtio_net_hdr);
>>>>        unsigned int features;
>>>>
>>>> -    fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
>>>> +
>>>> +    ret = if_nametoindex(ifname);
>>>> +    if (ret) {
>>>> +        g_autofree char *file = g_strdup_printf("/dev/tap%d", ret);
>>>> +        fd = open(file, O_RDWR);
>>>> +    } else {
>>>> +        fd = -1;
>>>> +    }
>>>> +
>>>>        if (fd < 0) {
>>>> -        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
>>>> -        return -1;
>>>> +        fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
>>>
>>> Any reason tuntap were tried after the macvtap/ipvtap?
>>
>> If we try tuntap first, we will know that it is not tuntap when calling
>> TUNSETIFF. We will need to call TUNGETFEATURES and TUNSETVNETHDRSZ again
>> in such a case because they precede TUNSETIFF. Calling them twice is
>> troublesome.
> 
> I may miss something, we are only at the phase of open() not TUNSETIFF?

We can tell if it is macvtap/ipvtap just by trying opening the device 
file. That is not possible with tuntap because tuntap uses /dev/net/tun, 
a device file common for all tuntap interfaces and its presence does not 
tell if the interface is tuntap.

> 
>>
>> This is also consistent with libvirt. libvirt first checks if
>> g_strdup_printf("/dev/tap%d", ifindex) exists, and falls back to tuntap
>> otherwise.
> 
> This is not what I understand from how layered products work. Libvirt
> should align with Qemu for low level things like TAP, not the reverse.

This change is intended for the use case where libvirt is not in use. In 
particular, I use mkosi, which is not a full fledged layering mechanism.

Regards,
Akihiko Odaki