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>

4 files changed, 44 insertions, 19 deletions

diff --git a/AUTHORS b/AUTHORS
index ea346c360..957bfce3b 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -77,7 +77,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 713049aab..3bdbbd73c 100644
--- 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,8 +275,9 @@ 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;
@@ -283,11 +285,29 @@ nl_sock_send__(struct nl_sock *sock, const struct ofpbuf *msg, bool wait)
 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 e74bc9837..78dd7b23a 100644
--- 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 6c19edb01..435907a52 100644
--- 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);