[dpdk-dev] [PATCH v2] net/af_packet: fix vlan_insert corruption

[Ferruh Yigit]

On 4/16/2019 4:13 PM, Bruce Richardson wrote:
> On Tue, Apr 16, 2019 at 08:03:36AM -0700, Stephen Hemminger wrote:
>> On Tue, 16 Apr 2019 10:42:13 +0100
>> Bruce Richardson <bruce.richardson at intel.com> wrote:
>>
>>> On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote:
>>>> On 4/12/2019 11:08 PM, Stephen Hemminger wrote:  
>>>>> On Fri, 12 Apr 2019 17:28:17 +0100
>>>>> Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>>>>>   
>>>>>> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:  
>>>>>>> If the af_packet transmit is sending a VLAN packet,
>>>>>>> and the transmit path to the kernel os full, then it would
>>>>>>> mismanage the outgoing mbuf. The original mbuf would end up
>>>>>>> being freed twice, once by AF_PACKET PMD and once by caller.    
>>>>>>
>>>>>> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
>>>>>> to duplicate the mbuf, right?
>>>>>>
>>>>>> That patch looks like won't make the release, so I suggest this one wait that
>>>>>> patch, although this is harmless on its own, commit log is misleading.
>>>>>>
>>>>>> [1]
>>>>>> https://patches.dpdk.org/patch/51870/  
>>>>>
>>>>> It was always true, even with existing vlan_insert.
>>>>> Existing vlan_insert has issues if it ever creates a clone packet.
>>>>> Existing vlan_insert can duplicate mbuf through clone
>>>>>   
>>>>
>>>> Right, existing vlan_insert has same issue on af_packet.
>>>>
>>>> But, should vlan_insert try to duplicate the mbuf when it is shared, does it
>>>> worth the complexity it brings? And when that support removed this patch won't
>>>> be needed.  
>>>
>>> I don't think vlan insert or other mbuf manipulation APIs should be
>>> checking for shared state or not - that's the job of the app. We could have
>>> cases where the user does want to modify a shared mbuf.
>>>
>>> Regards,
>>> /Bruce
>>
>> The vlan_insert code is called on transmit, and there are lots of
>> cases where a transmit mbuf might be shared (like TCP stack). And in that
>> case inserting the vlan must be non-destructive to the original mbuf.
>>
>> Whether you want to push the problem to the driver or do it in the
>> library, it is still a problem.
> 
> Yes, I agree it's a problem. I'd prefer see it done in the driver than in the
> library, since it's higher in the SW stack and has more context information
> as to what is safe or not.

The patch not more needed with current rte_vlan_insert()