[ovs-dev] [datapath minor fixes 2/3] datapath: avoid double free routing entry in vxlan_port xmit

[Andy Zhou]

On Wed, Oct 1, 2014 at 1:38 PM, Jesse Gross <jesse at nicira.com> wrote:
> I agree that the memory handling is too fragile here. Maybe we should
> just make people attach the dst before calling into tunneling routines
> though. It might be easier considering the multiple entry points (i.e.
> GRE calls iptunnel_xmit() directly).

Yes, this is a better approach. I will fix up the Geneve driver before upstream.
I will come up with separate patch for Vxlan.

>
> Several callers are definitely expecting iptunnel_xmit() to
> potentially return an error code. It's probably less confusing to not
> buck that trend and keep it as is. I guess in theory it could return
> an error code, it just doesn't right now.
Good point. However, if iptunnel_xmit() could return error code, then
above fix is a must.
>
> On Wed, Oct 1, 2014 at 1:11 PM, Andy Zhou <azhou at nicira.com> wrote:
>> It turns out rout entry will not be attached to skb until
>> iptunnel_xmit, which does not return negative value. So there is no
>> bug here.
>>
>> I will drop this patch, and modify the upstream vport-geneve.c accordingly.
>>
>> It is pretty subtle that the correctness of free depends on the
>> implementation details of the API.  May be it will be better for
>> vxlan_xmit_skb always
>> attach the route skb at the very beginning?
>>
>> Should we also make iptunnel_xmit() return type as unsigned int?
>>
>> On Wed, Oct 1, 2014 at 10:42 AM, Jesse Gross <jesse at nicira.com> wrote:
>>> On Wed, Oct 1, 2014 at 1:02 AM, Andy Zhou <azhou at nicira.com> wrote:
>>>> Route entry will be free on error by vport when freeing skb.
>>>> additional error check and free after xmit() will cause double free.
>>>>
>>>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>>>
>>> I'm not sure that the route entry is attached to the skb in all cases.
>>> If it's not then, this patch will cause a leak in those situations.