util: set src_pid for virNetlinkCommand when appropriate

Until now, the nl_pid of the source address of every message sent by
virNetlinkCommand has been set to the value of getpid(). Most of the
time this doesn't matter, and in the one case where it does
(communication with lldpad), it previously was the proper thing to do,
because the netlink event service (which listens on a netlink socket
for unsolicited messages from lldpad) coincidentally always happened
to bind with a local nl_pid == getpid().

With the fix for:

  https://bugzilla.redhat.com/show_bug.cgi?id=816465

that particular nl_pid is now effectively a reserved value, so the
netlink event service will always bind to something else
(coincidentally "getpid() + (1 << 22)", but it really could be
anything). The result is that communication between lldpad and
libvirtd is broken (lldpad gets a "disconnected" error when it tries
to send a directed message).

The solution to this problem caused by a solution, is to query the
netlink event service's nlhandle for its "local_port", and send that
as the source nl_pid (but only when sending to lldpad, of course - in
other cases we maintain the old behavior of sending getpid()).

There are two cases where a message is being directed at lldpad - one
in virNetDevLinkDump, and one in virNetDevVPortProfileOpSetLink.

The case of virNetDevVPortProfileOpSetLink is simplest to explain -
only if !nltarget_kernel, i.e. the message isn't targetted for the
kernel, is the dst_pid set (by calling
virNetDevVPortProfileGetLldpadPid()), so only in that case do we call
virNetlinkEventServiceLocalPid() to set src_pid.

For virNetDevLinkDump, it's a bit more complicated. The call to
virNetDevVPortProfileGetLldpadPid() was effectively up one level (in
virNetDevVPortProfileOpCommon), although obscured by an unnecessary
passing of a function pointer. This patch removes the function
pointer, and calls virNetDevVPortProfileGetLldpadPid() directly in
virNetDevVPortProfileOpCommon - if it's doing this, it knows that it
should also call virNetlinkEventServiceLocalPid() to set src_pid too;
then it just passes src_pid and dst_pid down to
virNetDevLinkDump. Since (src_pid == 0 && dst_pid == 0) implies that
the kernel is the destination, there is no longer any need to send
nltarget_kernel as an arg to virNetDevLinkDump, so it's been removed.

The disparity between src_pid being int and dst_pid being uint32_t may
be a bit disconcerting to some, but I didn't want to complicate
virNetlinkEventServiceLocalPid() by having status returned separately
from the value.

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 72af3cd778e928ee49ef5104de69d872520a000c..d53352f39c882df9afdd3477aba24ac3651c73be 100644 (file)
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1229,15 +1229,15 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
  *
  * @ifname: The name of the interface; only use if ifindex < 0
  * @ifindex: The interface index; may be < 0 if ifname is given
- * @nltarget_kernel: whether to send the message to the kernel or another
- *                   process
  * @nlattr: pointer to a pointer of netlink attributes that will contain
  *          the results
  * @recvbuf: Pointer to the buffer holding the returned netlink response
  *           message; free it, once not needed anymore
- * @getPidFunc: Pointer to a function that will be invoked if the kernel
- *              is not the target of the netlink message but it is to be
- *              sent to another process.
+ * @src_pid: pid used for nl_pid of the local end of the netlink message
+ *           (0 == "use getpid()")
+ * @dst_pid: pid of destination nl_pid if the kernel
+ *           is not the target of the netlink message but it is to be
+ *           sent to another process (0 if sending to the kernel)
  *
  * Get information about an interface given its name or index.
  *
@@ -1245,9 +1245,9 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
  */
 int
 virNetDevLinkDump(const char *ifname, int ifindex,
-                  bool nltarget_kernel, struct nlattr **tb,
+                  struct nlattr **tb,
                   unsigned char **recvbuf,
-                  uint32_t (*getPidFunc)(void))
+                  uint32_t src_pid, uint32_t dst_pid)
 {
     int rc = -1;
     struct nlmsghdr *resp;
@@ -1257,7 +1257,6 @@ virNetDevLinkDump(const char *ifname, int ifindex,
         .ifi_index  = ifindex
     };
     unsigned int recvbuflen;
-    uint32_t pid = 0;
     struct nl_msg *nl_msg;
 
     *recvbuf = NULL;
@@ -1281,14 +1280,7 @@ virNetDevLinkDump(const char *ifname, int ifindex,
             goto buffer_too_small;
     }
 
-    if (!nltarget_kernel) {
-        pid = getPidFunc();
-        if (pid == 0) {
-            goto cleanup;
-        }
-    }
-
-    if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, 0, pid) < 0)
+    if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, src_pid, dst_pid) < 0)
         goto cleanup;
 
     if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf == NULL)
@@ -1526,7 +1518,7 @@ virNetDevGetVfConfig(const char *ifname, int vf, unsigned char *mac,
     struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
     int ifindex = -1;
 
-    rc = virNetDevLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL);
+    rc = virNetDevLinkDump(ifname, ifindex, tb, &recvbuf, 0, 0);
     if (rc < 0)
         return rc;
 
@@ -1658,10 +1650,10 @@ virNetDevRestoreNetConfig(char *linkdev, int vf, char *stateDir)
 int
 virNetDevLinkDump(const char *ifname ATTRIBUTE_UNUSED,
                   int ifindex ATTRIBUTE_UNUSED,
-                  bool nltarget_kernel ATTRIBUTE_UNUSED,
                   struct nlattr **tb ATTRIBUTE_UNUSED,
                   unsigned char **recvbuf ATTRIBUTE_UNUSED,
-                  uint32_t (*getPidFunc)(void) ATTRIBUTE_UNUSED)
+                  uint32_t src_pid ATTRIBUTE_UNUSED,
+                  uint32_t dst_pid ATTRIBUTE_UNUSED)
 {
     virReportSystemError(ENOSYS, "%s",
                          _("Unable to dump link info on this platform"));

diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 73f4c64107cb20fa6bb449bdb1f197b5979ac5c1..660d2db53f12e2d1cfb2c16f87ee6b1925b61d3a 100644 (file)
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -107,9 +107,9 @@ int virNetDevGetVirtualFunctions(const char *pfname,
     ATTRIBUTE_RETURN_CHECK;
 
 int virNetDevLinkDump(const char *ifname, int ifindex,
-                      bool nltarget_kernel, struct nlattr **tb,
+                      struct nlattr **tb,
                       unsigned char **recvbuf,
-                      uint32_t (*getPidFunc)(void))
+                      uint32_t src_pid, uint32_t dst_pid)
     ATTRIBUTE_RETURN_CHECK;
 
 int virNetDevReplaceNetConfig(char *linkdev, int vf,

diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index def5c620314473f327ae097184272354da28ed22..38261d11858d19ec57b32139a2e78454a3fa878e 100644 (file)
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -282,7 +282,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
     };
     unsigned char *recvbuf = NULL;
     unsigned int recvbuflen = 0;
-    uint32_t pid = 0;
+    int src_pid = 0;
+    uint32_t dst_pid = 0;
     struct nl_msg *nl_msg;
     struct nlattr *vfports = NULL, *vfport;
 
@@ -390,12 +391,13 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
     }
 
     if (!nltarget_kernel) {
-        pid = virNetDevVPortProfileGetLldpadPid();
-        if (pid == 0)
+        if ((src_pid = virNetlinkEventServiceLocalPid()) < 0)
+            goto cleanup;
+        if ((dst_pid = virNetDevVPortProfileGetLldpadPid()) == 0)
             goto cleanup;
     }
 
-    if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0, pid) < 0)
+    if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, src_pid, dst_pid) < 0)
         goto cleanup;
 
     if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL)
@@ -477,7 +479,7 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int
         return -1;
 
     while (!end && i <= nthParent) {
-        rc = virNetDevLinkDump(ifname, ifindex, true, tb, &recvbuf, NULL);
+        rc = virNetDevLinkDump(ifname, ifindex, tb, &recvbuf, 0, 0);
         if (rc < 0)
             break;
 
@@ -524,6 +526,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
                               bool setlink_only)
 {
     int rc;
+    int src_pid = 0;
+    uint32_t dst_pid = 0;
     unsigned char *recvbuf = NULL;
     struct nlattr *tb[IFLA_MAX + 1] = { NULL , };
     int repeats = STATUS_POLL_TIMEOUT_USEC / STATUS_POLL_INTERVL_USEC;
@@ -549,9 +553,15 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
     if (setlink_only) /*for re-associations on existing links*/
         return 0;
 
+    if (!nltarget_kernel &&
+        (((src_pid = virNetlinkEventServiceLocalPid()) < 0) ||
+         ((dst_pid = virNetDevVPortProfileGetLldpadPid()) == 0))) {
+        rc = -1;
+        goto cleanup;
+    }
+
     while (--repeats >= 0) {
-        rc = virNetDevLinkDump(NULL, ifindex, nltarget_kernel, tb,
-                               &recvbuf, virNetDevVPortProfileGetLldpadPid);
+        rc = virNetDevLinkDump(NULL, ifindex, tb, &recvbuf, src_pid, dst_pid);
         if (rc < 0)
             goto cleanup;