On Tue, Nov 4, 2014 at 12:39 PM, Andy Zhou <az...@nicira.com> wrote:
> Looks good. Acked-by: Andy Zhou <az...@nicira.com>
>
> It would be to nice add more details to the commit message. The actual
> problem occurs when ovs_dp_name() is called.
>
I pushed this fix to master.
> In the long run, it would be nice to add lock depends to
> lookup_datapath. It seems all the use cases requires mutex to be held
> to prevent related data structures, such as vport list, from changing.
>
I agree, I will send followup patch.
Thanks.
> On Wed, Oct 29, 2014 at 2:45 AM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> dp read operations depends on ovs_dp_cmd_fill_info(). This API
>> needs to looup vport to find dp name, but vport lookup can
>> fail. Therefore to keep vport reference alive we need to
>> take ovs lock.
>>
>> Found by code inspection.
>>
>> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
>> ---
>> datapath/datapath.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 6820a95..5fc036c 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -1332,7 +1332,7 @@ static size_t ovs_dp_cmd_msg_size(void)
>> return msgsize;
>> }
>>
>> -/* Called with ovs_mutex or RCU read lock. */
>> +/* Called with ovs_mutex. */
>> static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
>> u32 portid, u32 seq, u32 flags, u8 cmd)
>> {
>> @@ -1618,7 +1618,7 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct
>> genl_info *info)
>> if (!reply)
>> return -ENOMEM;
>>
>> - rcu_read_lock();
>> + ovs_lock();
>> dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
>> if (IS_ERR(dp)) {
>> err = PTR_ERR(dp);
>> @@ -1627,7 +1627,7 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct
>> genl_info *info)
>> err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>> info->snd_seq, 0, OVS_DP_CMD_NEW);
>> BUG_ON(err < 0);
>> - rcu_read_unlock();
>> + ovs_unlock();
>>
>> return genlmsg_reply(reply, info);
>>
>> @@ -1644,8 +1644,8 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct
>> netlink_callback *cb)
>> int skip = cb->args[0];
>> int i = 0;
>>
>> - rcu_read_lock();
>> - list_for_each_entry_rcu(dp, &ovs_net->dps, list_node) {
>> + ovs_lock();
>> + list_for_each_entry(dp, &ovs_net->dps, list_node) {
>> if (i >= skip &&
>> ovs_dp_cmd_fill_info(dp, skb, NETLINK_CB(cb->skb).portid,
>> cb->nlh->nlmsg_seq, NLM_F_MULTI,
>> @@ -1653,7 +1653,7 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct
>> netlink_callback *cb)
>> break;
>> i++;
>> }
>> - rcu_read_unlock();
>> + ovs_unlock();
>>
>> cb->args[0] = i;
>>
>> --
>> 1.9.3
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
