datapath: Check locks on access to flow table.
[sliver-openvswitch.git] / datapath / datapath.c
index 65d25c2..796c40c 100644 (file)
@@ -67,7 +67,7 @@ EXPORT_SYMBOL(dp_ioctl_hook);
  * It is safe to access the datapath and vport structures with just
  * dp_mutex.
  */
-static struct datapath *dps[ODP_MAX];
+static struct datapath __rcu *dps[ODP_MAX];
 static DEFINE_MUTEX(dp_mutex);
 
 static int new_vport(struct datapath *, struct odp_port *, int port_no);
@@ -77,7 +77,8 @@ struct datapath *get_dp(int dp_idx)
 {
        if (dp_idx < 0 || dp_idx >= ODP_MAX)
                return NULL;
-       return rcu_dereference(dps[dp_idx]);
+       return rcu_dereference_check(dps[dp_idx], rcu_read_lock_held() ||
+                                        lockdep_is_held(&dp_mutex));
 }
 EXPORT_SYMBOL_GPL(get_dp);
 
@@ -93,10 +94,15 @@ static struct datapath *get_dp_locked(int dp_idx)
        return dp;
 }
 
+static struct tbl *get_table_protected(struct datapath *dp)
+{
+       return rcu_dereference_protected(dp->table, lockdep_is_held(&dp->mutex));
+}
+
 /* Must be called with rcu_read_lock or RTNL lock. */
 const char *dp_name(const struct datapath *dp)
 {
-       return vport_get_name(dp->ports[ODPP_LOCAL]);
+       return vport_get_name(rcu_dereference_rtnl(dp->ports[ODPP_LOCAL]));
 }
 
 static inline size_t br_nlmsg_size(void)
@@ -139,7 +145,8 @@ static int dp_fill_ifinfo(struct sk_buff *skb,
        hdr->ifi_change = 0;
 
        NLA_PUT_STRING(skb, IFLA_IFNAME, vport_get_name(port));
-       NLA_PUT_U32(skb, IFLA_MASTER, vport_get_ifindex(dp->ports[ODPP_LOCAL]));
+       NLA_PUT_U32(skb, IFLA_MASTER,
+               vport_get_ifindex(rtnl_dereference(dp->ports[ODPP_LOCAL])));
        NLA_PUT_U32(skb, IFLA_MTU, vport_get_mtu(port));
 #ifdef IFLA_OPERSTATE
        NLA_PUT_U8(skb, IFLA_OPERSTATE,
@@ -267,11 +274,11 @@ static int create_dp(int dp_idx, const char __user *devnamep)
                goto err_destroy_local_port;
 
        rcu_assign_pointer(dps[dp_idx], dp);
+       dp_sysfs_add_dp(dp);
+
        mutex_unlock(&dp_mutex);
        rtnl_unlock();
 
-       dp_sysfs_add_dp(dp);
-
        return 0;
 
 err_destroy_local_port:
@@ -493,7 +500,8 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
                flow_node = tbl_lookup(rcu_dereference(dp->table), &key,
                                        flow_hash(&key), flow_cmp);
                if (unlikely(!flow_node)) {
-                       dp_output_control(dp, skb, _ODPL_MISS_NR, OVS_CB(skb)->tun_id);
+                       dp_output_control(dp, skb, _ODPL_MISS_NR,
+                                        (__force u64)OVS_CB(skb)->tun_id);
                        stats_counter_off = offsetof(struct dp_stats_percpu, n_missed);
                        goto out;
                }
@@ -501,6 +509,7 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
                OVS_CB(skb)->flow = flow_cast(flow_node);
        }
 
+       stats_counter_off = offsetof(struct dp_stats_percpu, n_hit);
        flow_used(OVS_CB(skb)->flow, skb);
 
        acts = rcu_dereference(OVS_CB(skb)->flow->sf_acts);
@@ -511,13 +520,13 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
                loop->looping = true;
        if (unlikely(loop->looping)) {
                loop_suppress(dp, acts);
+               kfree_skb(skb);
                goto out_loop;
        }
 
        /* Execute actions. */
        execute_actions(dp, skb, &OVS_CB(skb)->flow->key, acts->actions,
-                       acts->n_actions);
-       stats_counter_off = offsetof(struct dp_stats_percpu, n_hit);
+                       acts->actions_len);
 
        /* Check whether sub-actions looped too much. */
        if (unlikely(loop->looping))
@@ -544,7 +553,7 @@ out:
 /* Append each packet in 'skb' list to 'queue'.  There will be only one packet
  * unless we broke up a GSO packet. */
 static int queue_control_packets(struct sk_buff *skb, struct sk_buff_head *queue,
-                                int queue_no, u32 arg)
+                                int queue_no, u64 arg)
 {
        struct sk_buff *nskb;
        int port_no;
@@ -569,7 +578,6 @@ static int queue_control_packets(struct sk_buff *skb, struct sk_buff_head *queue
                header->type = queue_no;
                header->length = skb->len;
                header->port = port_no;
-               header->reserved = 0;
                header->arg = arg;
                skb_queue_tail(queue, skb);
 
@@ -587,7 +595,7 @@ err_kfree_skbs:
 }
 
 int dp_output_control(struct datapath *dp, struct sk_buff *skb, int queue_no,
-                     u32 arg)
+                     u64 arg)
 {
        struct dp_stats_percpu *stats;
        struct sk_buff_head *queue;
@@ -610,16 +618,12 @@ int dp_output_control(struct datapath *dp, struct sk_buff *skb, int queue_no,
         * userspace may try to stuff a 64kB packet into a 1500-byte MTU. */
        if (skb_is_gso(skb)) {
                struct sk_buff *nskb = skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM);
-               if (nskb) {
-                       kfree_skb(skb);
-                       skb = nskb;
-                       if (unlikely(IS_ERR(skb))) {
-                               err = PTR_ERR(skb);
-                               goto err;
-                       }
-               } else {
-                       /* XXX This case might not be possible.  It's hard to
-                        * tell from the skb_gso_segment() code and comment. */
+               
+               kfree_skb(skb);
+               skb = nskb;
+               if (IS_ERR(skb)) {
+                       err = PTR_ERR(skb);
+                       goto err;
                }
        }
 
@@ -644,7 +648,7 @@ err:
 
 static int flush_flows(struct datapath *dp)
 {
-       struct tbl *old_table = rcu_dereference(dp->table);
+       struct tbl *old_table = get_table_protected(dp);
        struct tbl *new_table;
 
        new_table = tbl_create(0);
@@ -658,50 +662,76 @@ static int flush_flows(struct datapath *dp)
        return 0;
 }
 
-static int validate_actions(const struct sw_flow_actions *actions)
+static int validate_actions(const struct nlattr *actions, u32 actions_len)
 {
-       unsigned int i;
-
-       for (i = 0; i < actions->n_actions; i++) {
-               const union odp_action *a = &actions->actions[i];
-
-               switch (a->type) {
-               case ODPAT_CONTROLLER:
-               case ODPAT_STRIP_VLAN:
-               case ODPAT_SET_DL_SRC:
-               case ODPAT_SET_DL_DST:
-               case ODPAT_SET_NW_SRC:
-               case ODPAT_SET_NW_DST:
-               case ODPAT_SET_TP_SRC:
-               case ODPAT_SET_TP_DST:
-               case ODPAT_SET_TUNNEL:
-               case ODPAT_SET_PRIORITY:
-               case ODPAT_POP_PRIORITY:
-               case ODPAT_DROP_SPOOFED_ARP:
-                       /* No validation needed. */
-                       break;
+        const struct nlattr *a;
+        int rem;
+
+        nla_for_each_attr(a, actions, actions_len, rem) {
+                static const u32 action_lens[ODPAT_MAX + 1] = {
+                        [ODPAT_OUTPUT] = 4,
+                        [ODPAT_CONTROLLER] = 8,
+                        [ODPAT_SET_DL_TCI] = 2,
+                        [ODPAT_STRIP_VLAN] = 0,
+                        [ODPAT_SET_DL_SRC] = ETH_ALEN,
+                        [ODPAT_SET_DL_DST] = ETH_ALEN,
+                        [ODPAT_SET_NW_SRC] = 4,
+                        [ODPAT_SET_NW_DST] = 4,
+                        [ODPAT_SET_NW_TOS] = 1,
+                        [ODPAT_SET_TP_SRC] = 2,
+                        [ODPAT_SET_TP_DST] = 2,
+                        [ODPAT_SET_TUNNEL] = 8,
+                        [ODPAT_SET_PRIORITY] = 4,
+                        [ODPAT_POP_PRIORITY] = 0,
+                        [ODPAT_DROP_SPOOFED_ARP] = 0,
+                };
+                int type = nla_type(a);
+
+                if (type > ODPAT_MAX || nla_len(a) != action_lens[type])
+                        return -EINVAL;
+
+                switch (type) {
+               case ODPAT_UNSPEC:
+                       return -EINVAL;
 
-               case ODPAT_OUTPUT:
-                       if (a->output.port >= DP_MAX_PORTS)
+                case ODPAT_CONTROLLER:
+                case ODPAT_STRIP_VLAN:
+                case ODPAT_SET_DL_SRC:
+                case ODPAT_SET_DL_DST:
+                case ODPAT_SET_NW_SRC:
+                case ODPAT_SET_NW_DST:
+                case ODPAT_SET_TP_SRC:
+                case ODPAT_SET_TP_DST:
+                case ODPAT_SET_TUNNEL:
+                case ODPAT_SET_PRIORITY:
+                case ODPAT_POP_PRIORITY:
+                case ODPAT_DROP_SPOOFED_ARP:
+                        /* No validation needed. */
+                        break;
+
+                case ODPAT_OUTPUT:
+                        if (nla_get_u32(a) >= DP_MAX_PORTS)
+                                return -EINVAL;
+
+                case ODPAT_SET_DL_TCI:
+                       if (nla_get_be16(a) & htons(VLAN_CFI_MASK))
                                return -EINVAL;
-                       break;
+                        break;
 
-               case ODPAT_SET_DL_TCI:
-                       if (a->dl_tci.tci & htons(VLAN_CFI_MASK))
-                               return -EINVAL;
-                       break;
+                case ODPAT_SET_NW_TOS:
+                        if (nla_get_u8(a) & INET_ECN_MASK)
+                                return -EINVAL;
+                        break;
 
-               case ODPAT_SET_NW_TOS:
-                       if (a->nw_tos.nw_tos & INET_ECN_MASK)
-                               return -EINVAL;
-                       break;
+                default:
+                        return -EOPNOTSUPP;
+                }
+        }
 
-               default:
-                       return -EOPNOTSUPP;
-               }
-       }
+        if (rem > 0)
+                return -EINVAL;
 
-       return 0;
+        return 0;
 }
 
 static struct sw_flow_actions *get_actions(const struct odp_flow *flow)
@@ -709,16 +739,17 @@ static struct sw_flow_actions *get_actions(const struct odp_flow *flow)
        struct sw_flow_actions *actions;
        int error;
 
-       actions = flow_actions_alloc(flow->n_actions);
+       actions = flow_actions_alloc(flow->actions_len);
        error = PTR_ERR(actions);
        if (IS_ERR(actions))
                goto error;
 
        error = -EFAULT;
-       if (copy_from_user(actions->actions, flow->actions,
-                          flow->n_actions * sizeof(union odp_action)))
+       if (copy_from_user(actions->actions,
+                          (struct nlattr __user *)flow->actions,
+                          flow->actions_len))
                goto error_free_actions;
-       error = validate_actions(actions);
+       error = validate_actions(actions->actions, actions->actions_len);
        if (error)
                goto error_free_actions;
 
@@ -764,7 +795,7 @@ static void clear_stats(struct sw_flow *flow)
 
 static int expand_table(struct datapath *dp)
 {
-       struct tbl *old_table = rcu_dereference(dp->table);
+       struct tbl *old_table = get_table_protected(dp);
        struct tbl *new_table;
 
        new_table = tbl_expand(old_table);
@@ -785,7 +816,7 @@ static int do_put_flow(struct datapath *dp, struct odp_flow_put *uf,
        struct tbl *table;
        int error;
 
-       table = rcu_dereference(dp->table);
+       table = get_table_protected(dp);
        flow_node = tbl_lookup(table, &uf->flow.key, flow_hash(&uf->flow.key), flow_cmp);
        if (!flow_node) {
                /* No such flow. */
@@ -800,7 +831,7 @@ static int do_put_flow(struct datapath *dp, struct odp_flow_put *uf,
                        error = expand_table(dp);
                        if (error)
                                goto error;
-                       table = rcu_dereference(dp->table);
+                       table = get_table_protected(dp);
                }
 
                /* Allocate flow. */
@@ -842,9 +873,9 @@ static int do_put_flow(struct datapath *dp, struct odp_flow_put *uf,
                if (IS_ERR(new_acts))
                        goto error;
                old_acts = rcu_dereference(flow->sf_acts);
-               if (old_acts->n_actions != new_acts->n_actions ||
+               if (old_acts->actions_len != new_acts->actions_len ||
                    memcmp(old_acts->actions, new_acts->actions,
-                          sizeof(union odp_action) * old_acts->n_actions)) {
+                          old_acts->actions_len)) {
                        rcu_assign_pointer(flow->sf_acts, new_acts);
                        flow_deferred_free_acts(old_acts);
                } else {
@@ -892,12 +923,12 @@ static int put_flow(struct datapath *dp, struct odp_flow_put __user *ufp)
 
 static int do_answer_query(struct sw_flow *flow, u32 query_flags,
                           struct odp_flow_stats __user *ustats,
-                          union odp_action __user *actions,
-                          u32 __user *n_actionsp)
+                          struct nlattr __user *actions,
+                          u32 __user *actions_lenp)
 {
        struct sw_flow_actions *sf_acts;
        struct odp_flow_stats stats;
-       u32 n_actions;
+       u32 actions_len;
 
        spin_lock_bh(&flow->lock);
        get_stats(flow, &stats);
@@ -907,17 +938,16 @@ static int do_answer_query(struct sw_flow *flow, u32 query_flags,
        spin_unlock_bh(&flow->lock);
 
        if (copy_to_user(ustats, &stats, sizeof(struct odp_flow_stats)) ||
-           get_user(n_actions, n_actionsp))
+           get_user(actions_len, actions_lenp))
                return -EFAULT;
 
-       if (!n_actions)
+       if (!actions_len)
                return 0;
 
        sf_acts = rcu_dereference(flow->sf_acts);
-       if (put_user(sf_acts->n_actions, n_actionsp) ||
+       if (put_user(sf_acts->actions_len, actions_lenp) ||
            (actions && copy_to_user(actions, sf_acts->actions,
-                                    sizeof(union odp_action) *
-                                    min(sf_acts->n_actions, n_actions))))
+                                    min(sf_acts->actions_len, actions_len))))
                return -EFAULT;
 
        return 0;
@@ -926,18 +956,18 @@ static int do_answer_query(struct sw_flow *flow, u32 query_flags,
 static int answer_query(struct sw_flow *flow, u32 query_flags,
                        struct odp_flow __user *ufp)
 {
-       union odp_action *actions;
+       struct nlattr __user *actions;
 
-       if (get_user(actions, &ufp->actions))
+       if (get_user(actions, (struct nlattr __user * __user *)&ufp->actions))
                return -EFAULT;
 
        return do_answer_query(flow, query_flags, 
-                              &ufp->stats, actions, &ufp->n_actions);
+                              &ufp->stats, actions, &ufp->actions_len);
 }
 
 static struct sw_flow *do_del_flow(struct datapath *dp, struct odp_flow_key *key)
 {
-       struct tbl *table = rcu_dereference(dp->table);
+       struct tbl *table = get_table_protected(dp);
        struct tbl_node *flow_node;
        int error;
 
@@ -976,11 +1006,11 @@ static int del_flow(struct datapath *dp, struct odp_flow __user *ufp)
 
 static int do_query_flows(struct datapath *dp, const struct odp_flowvec *flowvec)
 {
-       struct tbl *table = rcu_dereference(dp->table);
+       struct tbl *table = get_table_protected(dp);
        u32 i;
 
        for (i = 0; i < flowvec->n_flows; i++) {
-               struct odp_flow __user *ufp = &flowvec->flows[i];
+               struct odp_flow __user *ufp = (struct odp_flow __user *)&flowvec->flows[i];
                struct odp_flow uf;
                struct tbl_node *flow_node;
                int error;
@@ -1031,11 +1061,11 @@ static int do_list_flows(struct datapath *dp, const struct odp_flowvec *flowvec)
        if (!flowvec->n_flows)
                return 0;
 
-       cbdata.uflows = flowvec->flows;
+       cbdata.uflows = (struct odp_flow __user *)flowvec->flows;
        cbdata.n_flows = flowvec->n_flows;
        cbdata.listed_flows = 0;
 
-       error = tbl_foreach(rcu_dereference(dp->table), list_flow, &cbdata);
+       error = tbl_foreach(get_table_protected(dp), list_flow, &cbdata);
        return error ? error : cbdata.listed_flows;
 }
 
@@ -1073,18 +1103,18 @@ static int do_execute(struct datapath *dp, const struct odp_execute *execute)
        if (execute->length < ETH_HLEN || execute->length > 65535)
                goto error;
 
-       actions = flow_actions_alloc(execute->n_actions);
+       actions = flow_actions_alloc(execute->actions_len);
        if (IS_ERR(actions)) {
                err = PTR_ERR(actions);
                goto error;
        }
 
        err = -EFAULT;
-       if (copy_from_user(actions->actions, execute->actions,
-                          execute->n_actions * sizeof *execute->actions))
+       if (copy_from_user(actions->actions,
+           (struct nlattr __user *)execute->actions, execute->actions_len))
                goto error_free_actions;
 
-       err = validate_actions(actions);
+       err = validate_actions(actions->actions, execute->actions_len);
        if (err)
                goto error_free_actions;
 
@@ -1094,7 +1124,8 @@ static int do_execute(struct datapath *dp, const struct odp_execute *execute)
                goto error_free_actions;
 
        err = -EFAULT;
-       if (copy_from_user(skb_put(skb, execute->length), execute->data,
+       if (copy_from_user(skb_put(skb, execute->length),
+                          (const void __user *)execute->data,
                           execute->length))
                goto error_free_skb;
 
@@ -1114,7 +1145,7 @@ static int do_execute(struct datapath *dp, const struct odp_execute *execute)
                goto error_free_skb;
 
        rcu_read_lock();
-       err = execute_actions(dp, skb, &key, actions->actions, actions->n_actions);
+       err = execute_actions(dp, skb, &key, actions->actions, actions->actions_len);
        rcu_read_unlock();
 
        kfree(actions);
@@ -1140,7 +1171,7 @@ static int execute_packet(struct datapath *dp, const struct odp_execute __user *
 
 static int get_dp_stats(struct datapath *dp, struct odp_stats __user *statsp)
 {
-       struct tbl *table = rcu_dereference(dp->table);
+       struct tbl *table = get_table_protected(dp);
        struct odp_stats stats;
        int i;
 
@@ -1298,7 +1329,8 @@ static int list_ports(struct datapath *dp, struct odp_portvec __user *upv)
        if (copy_from_user(&pv, upv, sizeof pv))
                return -EFAULT;
 
-       retval = do_list_ports(dp, pv.ports, pv.n_ports);
+       retval = do_list_ports(dp, (struct odp_port __user *)pv.ports,
+                              pv.n_ports);
        if (retval < 0)
                return retval;
 
@@ -1498,11 +1530,11 @@ static int compat_get_flow(struct odp_flow *flow, const struct compat_odp_flow _
            __copy_from_user(&flow->stats, &compat->stats, sizeof(struct odp_flow_stats)) ||
            __copy_from_user(&flow->key, &compat->key, sizeof(struct odp_flow_key)) ||
            __get_user(actions, &compat->actions) ||
-           __get_user(flow->n_actions, &compat->n_actions) ||
+           __get_user(flow->actions_len, &compat->actions_len) ||
            __get_user(flow->flags, &compat->flags))
                return -EFAULT;
 
-       flow->actions = compat_ptr(actions);
+       flow->actions = (struct nlattr __force *)compat_ptr(actions);
        return 0;
 }
 
@@ -1536,7 +1568,7 @@ static int compat_answer_query(struct sw_flow *flow, u32 query_flags,
                return -EFAULT;
 
        return do_answer_query(flow, query_flags, &ufp->stats,
-                              compat_ptr(actions), &ufp->n_actions);
+                              compat_ptr(actions), &ufp->actions_len);
 }
 
 static int compat_del_flow(struct datapath *dp, struct compat_odp_flow __user *ufp)
@@ -1557,9 +1589,11 @@ static int compat_del_flow(struct datapath *dp, struct compat_odp_flow __user *u
        return error;
 }
 
-static int compat_query_flows(struct datapath *dp, struct compat_odp_flow *flows, u32 n_flows)
+static int compat_query_flows(struct datapath *dp,
+                             struct compat_odp_flow __user *flows,
+                             u32 n_flows)
 {
-       struct tbl *table = rcu_dereference(dp->table);
+       struct tbl *table = get_table_protected(dp);
        u32 i;
 
        for (i = 0; i < n_flows; i++) {
@@ -1606,7 +1640,8 @@ static int compat_list_flow(struct tbl_node *node, void *cbdata_)
        return 0;
 }
 
-static int compat_list_flows(struct datapath *dp, struct compat_odp_flow *flows, u32 n_flows)
+static int compat_list_flows(struct datapath *dp,
+                            struct compat_odp_flow __user *flows, u32 n_flows)
 {
        struct compat_list_flows_cbdata cbdata;
        int error;
@@ -1618,13 +1653,13 @@ static int compat_list_flows(struct datapath *dp, struct compat_odp_flow *flows,
        cbdata.n_flows = n_flows;
        cbdata.listed_flows = 0;
 
-       error = tbl_foreach(rcu_dereference(dp->table), compat_list_flow, &cbdata);
+       error = tbl_foreach(get_table_protected(dp), compat_list_flow, &cbdata);
        return error ? error : cbdata.listed_flows;
 }
 
 static int compat_flowvec_ioctl(struct datapath *dp, unsigned long argp,
                                int (*function)(struct datapath *,
-                                               struct compat_odp_flow *,
+                                               struct compat_odp_flow __user *,
                                                u32 n_flows))
 {
        struct compat_odp_flowvec __user *uflowvec;
@@ -1659,13 +1694,13 @@ static int compat_execute(struct datapath *dp, const struct compat_odp_execute _
 
        if (!access_ok(VERIFY_READ, uexecute, sizeof(struct compat_odp_execute)) ||
            __get_user(actions, &uexecute->actions) ||
-           __get_user(execute.n_actions, &uexecute->n_actions) ||
+           __get_user(execute.actions_len, &uexecute->actions_len) ||
            __get_user(data, &uexecute->data) ||
            __get_user(execute.length, &uexecute->length))
                return -EFAULT;
 
-       execute.actions = compat_ptr(actions);
-       execute.data = compat_ptr(data);
+       execute.actions = (struct nlattr __force *)compat_ptr(actions);
+       execute.data = (const void __force *)compat_ptr(data);
 
        return do_execute(dp, &execute);
 }
@@ -1841,10 +1876,9 @@ fault:
 static ssize_t openvswitch_read(struct file *f, char __user *buf,
                                size_t nbytes, loff_t *ppos)
 {
-       /* XXX is there sufficient synchronization here? */
        int listeners = get_listen_mask(f);
        int dp_idx = iminor(f->f_dentry->d_inode);
-       struct datapath *dp = get_dp(dp_idx);
+       struct datapath *dp = get_dp_locked(dp_idx);
        struct sk_buff *skb;
        size_t copy_bytes, tot_copy_bytes;
        int retval;
@@ -1881,6 +1915,8 @@ static ssize_t openvswitch_read(struct file *f, char __user *buf,
                }
        }
 success:
+       mutex_unlock(&dp->mutex);
+
        copy_bytes = tot_copy_bytes = min_t(size_t, skb->len, nbytes);
 
        retval = 0;
@@ -1899,7 +1935,8 @@ success:
                                copy_bytes = csum_start;
                                csump = (__sum16 __user *)(buf + csum_start + csum_offset);
 
-                               BUG_ON((char *)csump + sizeof(__sum16) > buf + nbytes);
+                               BUG_ON((char __user *)csump + sizeof(__sum16) >
+                                       buf + nbytes);
                                put_user(csum_fold(csum), csump);
                        }
                } else
@@ -1907,7 +1944,7 @@ success:
        }
 
        if (!retval) {
-               struct iovec __user iov;
+               struct iovec iov;
 
                iov.iov_base = buf;
                iov.iov_len = copy_bytes;
@@ -1918,16 +1955,17 @@ success:
                retval = tot_copy_bytes;
 
        kfree_skb(skb);
+       return retval;
 
 error:
+       mutex_unlock(&dp->mutex);
        return retval;
 }
 
 static unsigned int openvswitch_poll(struct file *file, poll_table *wait)
 {
-       /* XXX is there sufficient synchronization here? */
        int dp_idx = iminor(file->f_dentry->d_inode);
-       struct datapath *dp = get_dp(dp_idx);
+       struct datapath *dp = get_dp_locked(dp_idx);
        unsigned int mask;
 
        if (dp) {
@@ -1935,6 +1973,7 @@ static unsigned int openvswitch_poll(struct file *file, poll_table *wait)
                poll_wait(file, &dp->waitqueue, wait);
                if (dp_has_packet_of_interest(dp, get_listen_mask(file)))
                        mask |= POLLIN | POLLRDNORM;
+               mutex_unlock(&dp->mutex);
        } else {
                mask = POLLIN | POLLRDNORM | POLLHUP;
        }
@@ -1942,14 +1981,12 @@ static unsigned int openvswitch_poll(struct file *file, poll_table *wait)
 }
 
 static struct file_operations openvswitch_fops = {
-       /* XXX .aio_read = openvswitch_aio_read, */
        .read  = openvswitch_read,
        .poll  = openvswitch_poll,
        .unlocked_ioctl = openvswitch_ioctl,
 #ifdef CONFIG_COMPAT
        .compat_ioctl = openvswitch_compat_ioctl,
 #endif
-       /* XXX .fasync = openvswitch_fasync, */
 };
 
 static int major;