ovs-brcompatd: Fix sending replies to kernel requests.

Commit 7d7447 (netlink: Postpone choosing sequence numbers until send
time.) broke ovs-brcompatd because it prevented userspace replies to
kernel requests from using the correct sequence numbers.  This commit fixes
it.

Atzm Watanabe found the root cause and provided an alternative patch to
avoid the problem.

Reported-by: André Ruß <andre.russ@hybris.com>
Reported-by: Atzm Watanabe <atzm@stratosphere.co.jp>
Tested-by: Atzm Watanabe <atzm@stratosphere.co.jp>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/AUTHORS b/AUTHORS
index dc98ebe173ee0907d4a2afdf8646a28aee8fc9e8..47f6ea316ac8ae348a7654eb48a633fdaf22ae4c 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -80,7 +80,9 @@ Alan Shieh              ashieh@nicira.com
 Alban Browaeys          prahal@yahoo.com
 Alex Yip                alex@nicira.com
 Alexey I. Froloff       raorn@altlinux.org
+André Ruß               andre.russ@hybris.com
 Andreas Beckmann        debian@abeckmann.de
+Atzm Watanabe           atzm@stratosphere.co.jp
 Ben Basler              bbasler@nicira.com
 Bob Ball                bob.ball@citrix.com
 Brad Hall               brad@nicira.com

diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
index 713049aab0ed2367b472eb646830002700d7b8ce..3bdbbd73c70775d05b6a38ae50ae30afe50dcd9a 100644 (file)
--- a/lib/netlink-socket.c
+++ b/lib/netlink-socket.c
@@ -253,13 +253,14 @@ nl_sock_leave_mcgroup(struct nl_sock *sock, unsigned int multicast_group)
 }
 
 static int
-nl_sock_send__(struct nl_sock *sock, const struct ofpbuf *msg, bool wait)
+nl_sock_send__(struct nl_sock *sock, const struct ofpbuf *msg,
+               uint32_t nlmsg_seq, bool wait)
 {
     struct nlmsghdr *nlmsg = nl_msg_nlmsghdr(msg);
     int error;
 
     nlmsg->nlmsg_len = msg->size;
-    nlmsg->nlmsg_seq = nl_sock_allocate_seq(sock, 1);
+    nlmsg->nlmsg_seq = nlmsg_seq;
     nlmsg->nlmsg_pid = sock->pid;
     do {
         int retval;
@@ -274,20 +275,39 @@ nl_sock_send__(struct nl_sock *sock, const struct ofpbuf *msg, bool wait)
 }
 
 /* Tries to send 'msg', which must contain a Netlink message, to the kernel on
- * 'sock'.  nlmsg_len in 'msg' will be finalized to match msg->size, and
- * nlmsg_pid will be set to 'sock''s pid, before the message is sent.
+ * 'sock'.  nlmsg_len in 'msg' will be finalized to match msg->size, nlmsg_pid
+ * will be set to 'sock''s pid, and nlmsg_seq will be initialized to a fresh
+ * sequence number, before the message is sent.
  *
  * Returns 0 if successful, otherwise a positive errno value.  If
  * 'wait' is true, then the send will wait until buffer space is ready;
  * otherwise, returns EAGAIN if the 'sock' send buffer is full. */
 int
 nl_sock_send(struct nl_sock *sock, const struct ofpbuf *msg, bool wait)
+{
+    return nl_sock_send_seq(sock, msg, nl_sock_allocate_seq(sock, 1), wait);
+}
+
+/* Tries to send 'msg', which must contain a Netlink message, to the kernel on
+ * 'sock'.  nlmsg_len in 'msg' will be finalized to match msg->size, nlmsg_pid
+ * will be set to 'sock''s pid, and nlmsg_seq will be initialized to
+ * 'nlmsg_seq', before the message is sent.
+ *
+ * Returns 0 if successful, otherwise a positive errno value.  If
+ * 'wait' is true, then the send will wait until buffer space is ready;
+ * otherwise, returns EAGAIN if the 'sock' send buffer is full.
+ *
+ * This function is suitable for sending a reply to a request that was received
+ * with sequence number 'nlmsg_seq'.  Otherwise, use nl_sock_send() instead. */
+int
+nl_sock_send_seq(struct nl_sock *sock, const struct ofpbuf *msg,
+                 uint32_t nlmsg_seq, bool wait)
 {
     int error = nl_sock_cow__(sock);
     if (error) {
         return error;
     }
-    return nl_sock_send__(sock, msg, wait);
+    return nl_sock_send__(sock, msg, nlmsg_seq, wait);
 }
 
 /* This stress option is useful for testing that OVS properly tolerates
@@ -770,7 +790,8 @@ nl_dump_start(struct nl_dump *dump,
     }
 
     nl_msg_nlmsghdr(request)->nlmsg_flags |= NLM_F_DUMP | NLM_F_ACK;
-    dump->status = nl_sock_send__(sock, request, true);
+    dump->status = nl_sock_send__(sock, request, nl_sock_allocate_seq(sock, 1),
+                                  true);
     dump->seq = nl_msg_nlmsghdr(request)->nlmsg_seq;
 }
 

diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
index e74bc98374f73572381dcc66a3f981970a633b45..78dd7b23a1375a22c796f7779eb0b1a4043ad3b3 100644 (file)
--- a/lib/netlink-socket.h
+++ b/lib/netlink-socket.h
@@ -52,6 +52,8 @@ int nl_sock_join_mcgroup(struct nl_sock *, unsigned int multicast_group);
 int nl_sock_leave_mcgroup(struct nl_sock *, unsigned int multicast_group);
 
 int nl_sock_send(struct nl_sock *, const struct ofpbuf *, bool wait);
+int nl_sock_send_seq(struct nl_sock *, const struct ofpbuf *,
+                     uint32_t nlmsg_seq, bool wait);
 int nl_sock_recv(struct nl_sock *, struct ofpbuf *, bool wait);
 int nl_sock_transact(struct nl_sock *, const struct ofpbuf *request,
                      struct ofpbuf **replyp);

diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c
index adabe89492538242ec6665bc7e8da0a0edec8ca4..df9332f3aaf38bc6e73b4f293392b4eed93864c2 100644 (file)
--- a/vswitchd/ovs-brcompatd.c
+++ b/vswitchd/ovs-brcompatd.c
@@ -280,25 +280,25 @@ parse_command(struct ofpbuf *buffer, uint32_t *seq, const char **br_name,
     return 0;
 }
 
-/* Composes and returns a reply to a request made by the datapath with Netlink
- * sequence number 'seq' and error code 'error'.  The caller may add additional
- * attributes to the message, then it may send it with send_reply(). */
+/* Composes and returns a reply to a request made by the datapath with error
+ * code 'error'.  The caller may add additional attributes to the message, then
+ * it may send it with send_reply(). */
 static struct ofpbuf *
-compose_reply(uint32_t seq, int error)
+compose_reply(int error)
 {
     struct ofpbuf *reply = ofpbuf_new(4096);
     nl_msg_put_genlmsghdr(reply, 32, brc_family, NLM_F_REQUEST,
                           BRC_GENL_C_DP_RESULT, 1);
-    ((struct nlmsghdr *) reply->data)->nlmsg_seq = seq;
     nl_msg_put_u32(reply, BRC_GENL_A_ERR_CODE, error);
     return reply;
 }
 
-/* Sends 'reply' to the datapath and frees it. */
+/* Sends 'reply' to the datapath, using sequence number 'nlmsg_seq', and frees
+ * it. */
 static void
-send_reply(struct ofpbuf *reply)
+send_reply(struct ofpbuf *reply, uint32_t nlmsg_seq)
 {
-    int retval = nl_sock_send(brc_sock, reply, false);
+    int retval = nl_sock_send_seq(brc_sock, reply, nlmsg_seq, false);
     if (retval) {
         VLOG_WARN_RL(&rl, "replying to brcompat request: %s",
                      strerror(retval));
@@ -311,7 +311,7 @@ send_reply(struct ofpbuf *reply)
 static void
 send_simple_reply(uint32_t seq, int error)
 {
-    send_reply(compose_reply(seq, error));
+    send_reply(compose_reply(error), seq);
 }
 
 static int
@@ -555,10 +555,10 @@ handle_fdb_query_cmd(struct ofpbuf *buffer)
     free(output);
 
     /* Compose and send reply to datapath. */
-    reply = compose_reply(seq, 0);
+    reply = compose_reply(0);
     nl_msg_put_unspec(reply, BRC_GENL_A_FDB_DATA,
                       query_data.data, query_data.size);
-    send_reply(reply);
+    send_reply(reply, seq);
 
     /* Free memory. */
     ofpbuf_uninit(&query_data);
@@ -594,10 +594,10 @@ send_ifindex_reply(uint32_t seq, char *output)
     }
 
     /* Compose and send reply. */
-    reply = compose_reply(seq, 0);
+    reply = compose_reply(0);
     nl_msg_put_unspec(reply, BRC_GENL_A_IFINDEXES,
                       indices, n_indices * sizeof *indices);
-    send_reply(reply);
+    send_reply(reply, seq);
 
     /* Free memory. */
     free(indices);