On Tue, Feb 4, 2014 at 11:10 AM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> ovs_vport_cmd_dump() did rcu_read_lock() only after getting the
> datapath, which could have been deleted in between. Resolved by
> taking rcu_read_lock() before the get_dp() call.
>
Good catch.
> Remove unnecessary locking from functions that are always called with
> appropriate locking (get_dp(), ovs_dp_cmd_fill_info(),
> lookup_datapath()).
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> datapath/datapath.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 5f1b34c..7992330 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -116,20 +116,19 @@ static int queue_gso_packets(struct datapath *dp,
> struct sk_buff *,
> static int queue_userspace_packet(struct datapath *dp, struct sk_buff *,
> const struct dp_upcall_info *);
>
> -/* Must be called with rcu_read_lock or ovs_mutex. */
> +/* Must be called with rcu_read_lock or ovs_mutex, so no additional
> protection
> + * is needed here. */
> static struct datapath *get_dp(struct net *net, int dp_ifindex)
> {
> struct datapath *dp = NULL;
> struct net_device *dev;
>
> - rcu_read_lock();
> dev = dev_get_by_index_rcu(net, dp_ifindex);
> if (dev) {
> struct vport *vport = ovs_internal_dev_get_vport(dev);
> if (vport)
> dp = vport->dp;
> }
> - rcu_read_unlock();
>
dev_get_by_index_rcu() needs RCU lock, so we can not remove this rcu-read-lock.
> return dp;
> }
> @@ -175,6 +174,7 @@ static struct hlist_head *vport_hash_bucket(const struct
> datapath *dp,
> return &dp->ports[port_no & (DP_VPORT_HASH_BUCKETS - 1)];
> }
>
> +/* Called with ovs_mutex or RCU read lock. */
> struct vport *ovs_lookup_vport(const struct datapath *dp, u16 port_no)
> {
> struct vport *vport;
> @@ -650,7 +650,7 @@ static size_t ovs_flow_cmd_msg_size(const struct
> sw_flow_actions *acts)
> + nla_total_size(acts->actions_len); /* OVS_FLOW_ATTR_ACTIONS
> */
> }
>
> -/* Called with ovs_mutex. */
> +/* Must be called with rcu_read_lock or ovs_mutex. */
> static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
> struct sk_buff *skb, u32 portid,
> u32 seq, u32 flags, u8 cmd)
> @@ -741,6 +741,7 @@ error:
> return err;
> }
>
> +/* Must be called with rcu_read_lock or ovs_mutex. */
> static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
> struct genl_info *info)
> {
> @@ -751,6 +752,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(struct
> sw_flow *flow,
> return genlmsg_new_unicast(len, info, GFP_KERNEL);
> }
>
> +/* Must be called with rcu_read_lock or ovs_mutex. */
> static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow,
> struct datapath *dp,
> struct genl_info *info,
> @@ -1091,6 +1093,7 @@ static size_t ovs_dp_cmd_msg_size(void)
> return msgsize;
> }
>
> +/* Must be called with rcu_read_lock or ovs_mutex. */
> static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
> u32 portid, u32 seq, u32 flags, u8 cmd)
> {
> @@ -1106,9 +1109,7 @@ static int ovs_dp_cmd_fill_info(struct datapath *dp,
> struct sk_buff *skb,
>
> ovs_header->dp_ifindex = get_dpifindex(dp);
>
> - rcu_read_lock();
> err = nla_put_string(skb, OVS_DP_ATTR_NAME, ovs_dp_name(dp));
> - rcu_read_unlock();
> if (err)
> goto nla_put_failure;
>
> @@ -1133,6 +1134,7 @@ error:
> return -EMSGSIZE;
> }
>
> +/* Must be called with rcu_read_lock or ovs_mutex. */
> static struct sk_buff *ovs_dp_cmd_build_info(struct datapath *dp,
> struct genl_info *info, u8 cmd)
> {
> @@ -1151,7 +1153,7 @@ static struct sk_buff *ovs_dp_cmd_build_info(struct
> datapath *dp,
> return skb;
> }
>
> -/* Called with ovs_mutex. */
> +/* Called with rcu_read_lock or ovs_mutex. */
> static struct datapath *lookup_datapath(struct net *net,
> struct ovs_header *ovs_header,
> struct nlattr *a[OVS_DP_ATTR_MAX + 1])
> @@ -1163,10 +1165,8 @@ static struct datapath *lookup_datapath(struct net
> *net,
> else {
> struct vport *vport;
>
> - rcu_read_lock();
> vport = ovs_vport_locate(net, nla_data(a[OVS_DP_ATTR_NAME]));
> dp = vport && vport->port_no == OVSP_LOCAL ? vport->dp : NULL;
> - rcu_read_unlock();
> }
> return dp ? dp : ERR_PTR(-ENODEV);
> }
> @@ -1764,11 +1764,12 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb,
> struct netlink_callback *cb)
> int bucket = cb->args[0], skip = cb->args[1];
> int i, j = 0;
>
> + rcu_read_lock();
> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> - if (!dp)
> + if (!dp) {
> + rcu_read_unlock();
> return -ENODEV;
> -
> - rcu_read_lock();
> + }
> for (i = bucket; i < DP_VPORT_HASH_BUCKETS; i++) {
> struct vport *vport;
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
