From ff459dd649b17f2a2613799c466e979ddd64cdf0 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 5 Jul 2012 08:41:03 -0700 Subject: [PATCH] ovs-brcompatd: Fix sending replies to kernel requests. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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ß Reported-by: Atzm Watanabe Tested-by: Atzm Watanabe Signed-off-by: Ben Pfaff --- AUTHORS | 2 ++ lib/netlink-socket.c | 33 +++++++++++++++++++++++++++------ lib/netlink-socket.h | 2 ++ vswitchd/ovs-brcompatd.c | 26 +++++++++++++------------- 4 files changed, 44 insertions(+), 19 deletions(-) diff --git a/AUTHORS b/AUTHORS index dc98ebe1..47f6ea31 100644 --- 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 713049aa..3bdbbd73 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,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 e74bc983..78dd7b23 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 adabe894..df9332f3 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); -- 2.30.2