[ovs-dev] [PATCH 1/4] ovs-vswitchd: Avoid segfault for "netdev" datapath.

[nickcooper-zhangtonghao]

Hi Ben and Daniele,

Thanks for your tips. I submitted the patch again. The test has been added.
May I also add “Signed-off-by: Daniele Di Proietto <diproiettod at ovn.org>” ?


Thanks.
Nick

> On Dec 7, 2016, at 7:51 AM, Daniele Di Proietto <diproiettod at ovn.org> wrote:
> 
> Thanks for the patch
> 
>>> ---
>>> lib/dpif-netdev.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 1400511..8175b7e 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -4380,11 +4380,12 @@ dp_execute_userspace_action(struct dp_netdev_pmd_thread *pmd,
>>>                             const struct nlattr *userdata, long long now)
>>> {
>>>     struct dp_packet_batch b;
>>> +    struct flow_wildcards wc;
>>>     int error;
>>> 
>>>     ofpbuf_clear(actions);
>>> 
>>> -    error = dp_netdev_upcall(pmd, packet, flow, NULL, ufid,
>>> +    error = dp_netdev_upcall(pmd, packet, flow, &wc, ufid,
>>>                              DPIF_UC_ACTION, userdata, actions,
>>>                              NULL);
>>>     if (!error || error == ENOSPC) {
>> 
>> I'm not too familiar with dpif-netdev.  However, there's probably some
>> reason that this wasn't done to start with; for example, it might be a
>> performance optimization of some kind.  Therefore, it's probably best to
>> at least consider fixing whatever function is doing the null
>> dereference.
> 
> I agree, it seems better to handle the case where 'wc' is NULL in upcall_cb().
> process_upcall() already handles it for non-miss upcalls, i.e. when coming from
> an OVS_ACTION_ATTR_USERSPACE datapath action.
> 
> Also, I think we could add a test, like the following: