6:23 p.m.
I found these while trying out some other code I decided not to use (I
was going to add more attributes to the <link> element,
virNetDevIfLink, and virNetDevGetLinkInfo(), but then decided not to
for now (in case Michal has a different idea of the meaning of "link"
than, e.g., iproute's "ip link" command).
The first two taken together actually have a very positive impact -
they make the output of "virsh nodedev-dumpxml" much more informative
when run as non-root.
Laine Stump (3):
util: use AF_UNIX family (not AF_PACKET) for ioctl sockets
util: allow retrieving ethtool features when unprivileged
network: remove unused typedef for networkDnsmasqLeaseFileNameFunc
src/network/bridge_driver.h | 2 --
src/util/virnetdev.c | 8 +-------
src/util/virnetdevip.c | 2 +-
3 files changed, 2 insertions(+), 10 deletions(-)
--
2.9.3
6:23 p.m.
New subject: [libvirt] [PATCH 1/3] util: use AF_UNIX family (not AF_PACKET) for ioctl sockets
The exact family of the socket created for the fd used by ioctl(7)
doesn't matter, it just needs to be a socket and not a file. But for
some reason when macvtap support was added, it used
AF_PACKET/SOCK_DGRAM sockets for its ioctls; we later used the same
AF_PACKET/SOCK_DGRAM socket for new ioctls we added, and eventually
modified the other pre-existing ioctl sockets (for creating/deleting
bridges) to also use AF_PACKET/SOCK_DGRAM (that code originally used
AF_UNIX/SOCK_STREAM).
The problem with using AF_PACKET (intended for sending/receiving "raw"
packets, i.e. packets that can be some protocol other than TCP or UDP)
is that it requires root privileges. This meant that none of the
ioctls in virnetdev.c or virnetdevip.c would work when running
libvirtd unprivileged.
This patch solves that problem by changing the family to AF_UNIX when
creating the socket used for any ioctl().
---
(Cc'ing Stefan Berger, since he originally added the code using
AF_PACKET, and I want to make sure this was just a random choice, and
not for some important reason I'm overlooking)
src/util/virnetdev.c | 2 +-
src/util/virnetdevip.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index d9f716b..b0159b2 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -41,7 +41,7 @@
#ifdef __linux__
# include <linux/sockios.h>
# include <linux/if_vlan.h>
-# define VIR_NETDEV_FAMILY AF_PACKET
+# define VIR_NETDEV_FAMILY AF_UNIX
#elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL)
# define VIR_NETDEV_FAMILY AF_LOCAL
#else
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 42fbba1..c82b8a5 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -44,7 +44,7 @@
#ifdef __linux__
# include <linux/sockios.h>
# include <linux/if_vlan.h>
-# define VIR_NETDEV_FAMILY AF_PACKET
+# define VIR_NETDEV_FAMILY AF_UNIX
#elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL)
# define VIR_NETDEV_FAMILY AF_LOCAL
#else
--
2.9.3
5:56 p.m.
New subject: [libvirt] [PATCH 1/3] util: use AF_UNIX family (not AF_PACKET) for ioctl sockets
On 03/21/2017 04:23 PM, Laine Stump wrote:
The exact family of the socket created for the fd used by ioctl(7)
doesn't matter, it just needs to be a socket and not a file. But for
some reason when macvtap support was added, it used
AF_PACKET/SOCK_DGRAM sockets for its ioctls; we later used the same
AF_PACKET/SOCK_DGRAM socket for new ioctls we added, and eventually
modified the other pre-existing ioctl sockets (for creating/deleting
bridges) to also use AF_PACKET/SOCK_DGRAM (that code originally used
AF_UNIX/SOCK_STREAM).
The problem with using AF_PACKET (intended for sending/receiving "raw"
packets, i.e. packets that can be some protocol other than TCP or UDP)
is that it requires root privileges. This meant that none of the
ioctls in virnetdev.c or virnetdevip.c would work when running
libvirtd unprivileged.
This patch solves that problem by changing the family to AF_UNIX when
creating the socket used for any ioctl().
---
(Cc'ing Stefan Berger, since he originally added the code using
AF_PACKET, and I want to make sure this was just a random choice, and
not for some important reason I'm overlooking)
src/util/virnetdev.c | 2 +-
src/util/virnetdevip.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index d9f716b..b0159b2 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -41,7 +41,7 @@
#ifdef __linux__
# include <linux/sockios.h>
# include <linux/if_vlan.h>
-# define VIR_NETDEV_FAMILY AF_PACKET
+# define VIR_NETDEV_FAMILY AF_UNIX
#elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL)
# define VIR_NETDEV_FAMILY AF_LOCAL
#else
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 42fbba1..c82b8a5 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -44,7 +44,7 @@
#ifdef __linux__
# include <linux/sockios.h>
# include <linux/if_vlan.h>
-# define VIR_NETDEV_FAMILY AF_PACKET
+# define VIR_NETDEV_FAMILY AF_UNIX
#elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL)
# define VIR_NETDEV_FAMILY AF_LOCAL
#else
ACK if you also remove the comment in virNetDevGetFeatures that mentions
AF_PACKET.
Michal
6:23 p.m.
New subject: [libvirt] [PATCH 2/3] util: allow retrieving ethtool features when unprivileged
The only reason that the ethtool features weren't being retrieved in
an unprivileged libvirtd was because they required ioctl(), and the
ioctl was using an AF_PACKET socket, which requires root. Now that we
are using AF_UNIX for ioctl(), this restriction can be removed.
---
src/util/virnetdev.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index b0159b2..0d19432 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -2980,12 +2980,6 @@ virNetDevGetFeatures(const char *ifname,
if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST)))
return -1;
- /* Only fetch features if we're privileged, but no need to fail */
- if (geteuid() != 0) {
- VIR_DEBUG("ETHTOOL feature bits not available in session mode");
- return 0;
- }
-
/* Ultimately uses AF_PACKET for socket which requires privileged
* daemon support.
*/
--
2.9.3
6:23 p.m.
New subject: [libvirt] [PATCH 3/3] network: remove unused typedef for networkDnsmasqLeaseFileNameFunc
---
src/network/bridge_driver.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index ff7f921..c696f03 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -102,6 +102,4 @@ networkBandwidthUpdate(virDomainNetDefPtr iface ATTRIBUTE_UNUSED,
# endif
-typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);
-
#endif /* __VIR_NETWORK__DRIVER_H */
--
2.9.3
5:56 p.m.
On 03/21/2017 04:23 PM, Laine Stump wrote:
I found these while trying out some other code I decided not to use
(I
was going to add more attributes to the <link> element,
virNetDevIfLink, and virNetDevGetLinkInfo(), but then decided not to
for now (in case Michal has a different idea of the meaning of "link"
than, e.g., iproute's "ip link" command).
The first two taken together actually have a very positive impact -
they make the output of "virsh nodedev-dumpxml" much more informative
when run as non-root.
Laine Stump (3):
util: use AF_UNIX family (not AF_PACKET) for ioctl sockets
util: allow retrieving ethtool features when unprivileged
network: remove unused typedef for networkDnsmasqLeaseFileNameFunc
src/network/bridge_driver.h | 2 --
src/util/virnetdev.c | 8 +-------
src/util/virnetdevip.c | 2 +-
3 files changed, 2 insertions(+), 10 deletions(-)
ACK series but see my comment to 1/3 before pushing please.
Michal
2950
days inactive
2951
days old
5 comments
2 participants
participants (2)
-
Laine Stump -
Michal Privoznik