[ovs-dev] [PATCH] dpif-linux: Don't reset kernel upcall_pids unintentionally.

Ben Pfaff blp at nicira.com
Sat Oct 8 02:54:30 UTC 2011


On Fri, Oct 07, 2011 at 06:25:57PM -0700, Jesse Gross wrote:
> On Fri, Oct 7, 2011 at 5:57 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Fri, Oct 07, 2011 at 05:05:15PM -0700, Jesse Gross wrote:
> >> On Fri, Oct 7, 2011 at 4:42 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > Commit b063d9f0 "datapath: Use unicast Netlink sockets for upcalls" that
> >> > introduced an 'upcall_pid' member into struct dpif_linux_vport, struct
> >> > dpif_linux_dp, and struct dpif_linux_flow neglected to do so only if the
> >> > member was nonzero. ??This caused every datapath, vport, and flow operation
> >> > to supply an upcall_pid. ??In particular, the netdev_set_config() called at
> >> > startup when a vport already existed caused the upcall_pid for that vport
> >> > to be reset to 0, which in turn caused all packets received on the vport to
> >> > be dropped instead of forwarded to ovs-vswitchd.
> >> >
> >> > Reported-by: Shih-Hao Li <shli at nicira.com>
> >>
> >> I think we actually want to distinguish between unset and zero. ??When
> >> the listen_mask indicates that a packet type shouldn't be received
> >> then we intentionally generate an upcall_pid of 0 to shut off those
> >> types of upcalls. ??Most of dpif-linux.c deals with this by simply
> >> always including the appropriate upcall_pid but that was missed for
> >> the calls in netdev-vport. ??At this point, nothing ever turns off
> >> parts of listen_mask, so it doesn't really matter but that was the
> >> intention.
> >
> > I actually understood these two cases as I wrote up the commit, but I
> > didn't see anything that currently needed to take advantage of it so I
> > ignored it.
> >
> > I can fix it up to separate "no change" and "set to zero", though, if
> > you prefer.
> 
> I guess it seems better to separate them out, otherwise the code is
> confusing because it's doing something in one place but ignoring it in
> another.

OK, here's v2.  Unlike the previous version, this one is compile-tested
only because I'm away from my desk.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Fri, 7 Oct 2011 16:41:36 -0700
Subject: [PATCH] dpif-linux: Don't reset kernel upcall_pids unintentionally.

Commit b063d9f0 "datapath: Use unicast Netlink sockets for upcalls" that
introduced an 'upcall_pid' member into struct dpif_linux_vport, struct
dpif_linux_dp, and struct dpif_linux_flow neglected to do so only if the
member was nonzero.  This caused every datapath, vport, and flow operation
to supply an upcall_pid.  In particular, the netdev_set_config() called at
startup when a vport already existed caused the upcall_pid for that vport
to be reset to 0, which in turn caused all packets received on the vport to
be dropped instead of forwarded to ovs-vswitchd.

Reported-by: Shih-Hao Li <shli at nicira.com>
---
 lib/dpif-linux.c |   48 +++++++++++++++++++++++++++++-------------------
 lib/dpif-linux.h |    2 +-
 2 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 8981500..0f188c6 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -77,7 +77,7 @@ struct dpif_linux_dp {
 
     /* Attributes. */
     const char *name;                  /* OVS_DP_ATTR_NAME. */
-    uint32_t upcall_pid;               /* OVS_DP_UPCALL_PID. */
+    const uint32_t *upcall_pid;        /* OVS_DP_UPCALL_PID. */
     struct ovs_dp_stats stats;         /* OVS_DP_ATTR_STATS. */
     enum ovs_frag_handling ipv4_frags; /* OVS_DP_ATTR_IPV4_FRAGS. */
 };
@@ -112,7 +112,7 @@ struct dpif_linux_flow {
     size_t key_len;
     const struct nlattr *actions;       /* OVS_FLOW_ATTR_ACTIONS. */
     size_t actions_len;
-    uint32_t upcall_pid;                /* OVS_FLOW_ATTR_UPCALL_PID. */
+    const uint32_t *upcall_pid;         /* OVS_FLOW_ATTR_UPCALL_PID. */
     const struct ovs_flow_stats *stats; /* OVS_FLOW_ATTR_STATS. */
     const uint8_t *tcp_flags;           /* OVS_FLOW_ATTR_TCP_FLAGS. */
     const uint64_t *used;               /* OVS_FLOW_ATTR_USED. */
@@ -415,16 +415,17 @@ dpif_linux_port_add(struct dpif *dpif_, struct netdev *netdev,
 
     /* Loop until we find a port that isn't used. */
     do {
+        uint32_t upcall_pid;
+
         request.port_no = dpif_linux_pop_port(dpif);
-        request.upcall_pid = get_upcall_pid_port(dpif, request.port_no);
+        upcall_pid = get_upcall_pid_port(dpif, request.port_no);
+        request.upcall_pid = &upcall_pid;
         error = dpif_linux_vport_transact(&request, &reply, &buf);
 
         if (!error) {
             *port_nop = reply.port_no;
-            VLOG_DBG("%s: assigning port %"PRIu32" to netlink "
-                     "pid %"PRIu32,
-                     dpif_name(dpif_), request.port_no,
-                     request.upcall_pid);
+            VLOG_DBG("%s: assigning port %"PRIu32" to netlink pid %"PRIu32,
+                     dpif_name(dpif_), request.port_no, upcall_pid);
         }
         ofpbuf_delete(buf);
     } while (request.port_no != UINT32_MAX
@@ -668,9 +669,12 @@ dpif_linux_flow_put(struct dpif *dpif_, enum dpif_flow_put_flags flags,
     struct dpif_linux *dpif = dpif_linux_cast(dpif_);
     struct dpif_linux_flow request, reply;
     struct nlattr dummy_action;
+    uint32_t upcall_pid;
     struct ofpbuf *buf;
     int error;
 
+    upcall_pid = get_upcall_pid_flow(dpif, key, key_len);
+
     dpif_linux_flow_init(&request);
     request.cmd = flags & DPIF_FP_CREATE ? OVS_FLOW_CMD_NEW : OVS_FLOW_CMD_SET;
     request.dp_ifindex = dpif->dp_ifindex;
@@ -679,7 +683,7 @@ dpif_linux_flow_put(struct dpif *dpif_, enum dpif_flow_put_flags flags,
     /* Ensure that OVS_FLOW_ATTR_ACTIONS will always be included. */
     request.actions = actions ? actions : &dummy_action;
     request.actions_len = actions_len;
-    request.upcall_pid = get_upcall_pid_flow(dpif, key, key_len);
+    request.upcall_pid = &upcall_pid;
     if (flags & DPIF_FP_ZERO_STATS) {
         request.clear = true;
     }
@@ -908,20 +912,19 @@ set_upcall_pids(struct dpif_linux *dpif)
     int error;
 
     DPIF_PORT_FOR_EACH (&port, &port_dump, &dpif->dpif) {
+        uint32_t upcall_pid = get_upcall_pid_port(dpif, port.port_no);
         struct dpif_linux_vport vport_request;
 
         dpif_linux_vport_init(&vport_request);
         vport_request.cmd = OVS_VPORT_CMD_SET;
         vport_request.dp_ifindex = dpif->dp_ifindex;
         vport_request.port_no = port.port_no;
-        vport_request.upcall_pid = get_upcall_pid_port(dpif,
-                                                       vport_request.port_no);
+        vport_request.upcall_pid = &upcall_pid;
         error = dpif_linux_vport_transact(&vport_request, NULL, NULL);
         if (!error) {
-            VLOG_DBG("%s: assigning port %"PRIu32" to netlink "
-                     "pid %"PRIu32,
+            VLOG_DBG("%s: assigning port %"PRIu32" to netlink pid %"PRIu32,
                      dpif_name(&dpif->dpif), vport_request.port_no,
-                     vport_request.upcall_pid);
+                     upcall_pid);
         } else {
             VLOG_WARN_RL(&error_rl, "%s: failed to set upcall pid on port: %s",
                          dpif_name(&dpif->dpif), strerror(error));
@@ -931,6 +934,7 @@ set_upcall_pids(struct dpif_linux *dpif)
     dpif_flow_dump_start(&flow_dump, &dpif->dpif);
     while (dpif_flow_dump_next(&flow_dump, &key, &key_len,
                                NULL, NULL, NULL)) {
+        uint32_t upcall_pid = get_upcall_pid_flow(dpif, key, key_len);
         struct dpif_linux_flow flow_request;
 
         dpif_linux_flow_init(&flow_request);
@@ -938,7 +942,7 @@ set_upcall_pids(struct dpif_linux *dpif)
         flow_request.dp_ifindex = dpif->dp_ifindex;
         flow_request.key = key;
         flow_request.key_len = key_len;
-        flow_request.upcall_pid = get_upcall_pid_flow(dpif, key, key_len);
+        flow_request.upcall_pid = &upcall_pid;
         error = dpif_linux_flow_transact(&flow_request, NULL, NULL);
         if (error) {
             VLOG_WARN_RL(&error_rl, "%s: failed to set upcall pid on flow: %s",
@@ -1326,7 +1330,7 @@ dpif_linux_vport_from_ofpbuf(struct dpif_linux_vport *vport,
     vport->type = nl_attr_get_u32(a[OVS_VPORT_ATTR_TYPE]);
     vport->name = nl_attr_get_string(a[OVS_VPORT_ATTR_NAME]);
     if (a[OVS_VPORT_ATTR_UPCALL_PID]) {
-        vport->upcall_pid = nl_attr_get_u32(a[OVS_VPORT_ATTR_UPCALL_PID]);
+        vport->upcall_pid = nl_attr_get(a[OVS_VPORT_ATTR_UPCALL_PID]);
     }
     if (a[OVS_VPORT_ATTR_STATS]) {
         vport->stats = nl_attr_get(a[OVS_VPORT_ATTR_STATS]);
@@ -1367,7 +1371,9 @@ dpif_linux_vport_to_ofpbuf(const struct dpif_linux_vport *vport,
         nl_msg_put_string(buf, OVS_VPORT_ATTR_NAME, vport->name);
     }
 
-    nl_msg_put_u32(buf, OVS_VPORT_ATTR_UPCALL_PID, vport->upcall_pid);
+    if (vport->upcall_pid) {
+        nl_msg_put_u32(buf, OVS_VPORT_ATTR_UPCALL_PID, *vport->upcall_pid);
+    }
 
     if (vport->stats) {
         nl_msg_put_unspec(buf, OVS_VPORT_ATTR_STATS,
@@ -1521,7 +1527,9 @@ dpif_linux_dp_to_ofpbuf(const struct dpif_linux_dp *dp, struct ofpbuf *buf)
         nl_msg_put_string(buf, OVS_DP_ATTR_NAME, dp->name);
     }
 
-    nl_msg_put_u32(buf, OVS_DP_ATTR_UPCALL_PID, dp->upcall_pid);
+    if (dp->upcall_pid) {
+        nl_msg_put_u32(buf, OVS_DP_ATTR_UPCALL_PID, *dp->upcall_pid);
+    }
 
     /* Skip OVS_DP_ATTR_STATS since we never have a reason to serialize it. */
 
@@ -1653,7 +1661,7 @@ dpif_linux_flow_from_ofpbuf(struct dpif_linux_flow *flow,
         flow->actions_len = nl_attr_get_size(a[OVS_FLOW_ATTR_ACTIONS]);
     }
     if (a[OVS_FLOW_ATTR_UPCALL_PID]) {
-        flow->upcall_pid = nl_attr_get_u32(a[OVS_FLOW_ATTR_UPCALL_PID]);
+        flow->upcall_pid = nl_attr_get(a[OVS_FLOW_ATTR_UPCALL_PID]);
     }
     if (a[OVS_FLOW_ATTR_STATS]) {
         flow->stats = nl_attr_get(a[OVS_FLOW_ATTR_STATS]);
@@ -1691,7 +1699,9 @@ dpif_linux_flow_to_ofpbuf(const struct dpif_linux_flow *flow,
                           flow->actions, flow->actions_len);
     }
 
-    nl_msg_put_u32(buf, OVS_FLOW_ATTR_UPCALL_PID, flow->upcall_pid);
+    if (flow->upcall_pid) {
+        nl_msg_put_u32(buf, OVS_FLOW_ATTR_UPCALL_PID, *flow->upcall_pid);
+    }
 
     /* We never need to send these to the kernel. */
     assert(!flow->stats);
diff --git a/lib/dpif-linux.h b/lib/dpif-linux.h
index f1a4faa..c72ea88 100644
--- a/lib/dpif-linux.h
+++ b/lib/dpif-linux.h
@@ -34,7 +34,7 @@ struct dpif_linux_vport {
 
     /* Attributes. */
     const char *name;                      /* OVS_VPORT_ATTR_NAME. */
-    uint32_t upcall_pid;                   /* OVS_VPORT_ATTR_UPCALL_PID. */
+    const uint32_t *upcall_pid;            /* OVS_VPORT_ATTR_UPCALL_PID. */
     const struct ovs_vport_stats *stats;   /* OVS_VPORT_ATTR_STATS. */
     const uint8_t *address;                /* OVS_VPORT_ATTR_ADDRESS. */
     const struct nlattr *options;          /* OVS_VPORT_ATTR_OPTIONS. */
-- 
1.7.4.4




More information about the dev mailing list