[ovs-dev] [RFC v3 1/5] datapath: Add a new action check_pkt_len

[Gregory Rose]

On 2/12/2019 1:15 PM, Gregory Rose wrote:
>
> On 2/11/2019 6:27 PM, Ben Pfaff wrote:
>> Greg, I recommend that you take a look at this one.
>
> My apologies but I haven't been feeling well the last few days. I'll 
> have a look at this one.
>
> Thanks,

I can't seem to find the actual patch - I went and looked at pipermail 
and I can't even find it there?  Only
replies.

If someone can point me to it or if Numan could post it again then I can 
review it.

Thanks,

- Greg

>
>>
>> (More commentary below.)
>>
>> On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq at redhat.com wrote:
>>> From: Numan Siddique <nusiddiq at redhat.com>
>>>
>>> [Please note, this patch is submitted as RFC in ovs-dev ML to
>>> get feedback before submitting to netdev ML]
>>>
>>> This patch adds a new action which checks the packet length and
>>> executes a set of actions if the packet length is greater than
>>> the specified length or executes another set of actions if the
>>> packet length is lesser or equal to.
>>>
>>> This action takes below nlattrs
>>>    * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
>>>
>>>    * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested 
>>> actions
>>>      to apply if the packet length is greater than the specified 
>>> 'pkt_len'
>>>
>>>    * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
>>>      actions to apply if the packet length is lesser or equal to the
>>>      specified 'pkt_len'.
>>>
>>> The main use case for adding this action is to solve the packet
>>> drops because of MTU mismatch in OVN virtual networking solution.
>>> When a VM (which belongs to a logical switch of OVN) sends a packet
>>> destined to go via the gateway router and if the nic which provides
>>> external connectivity, has a lesser MTU, OVS drops the packet
>>> if the packet length is greater than this MTU.
>>>
>>> With the help of this action, OVN will check the packet length
>>> and if it is greater than the MTU size, it will generate an
>>> ICMP packet (type 3, code 4) and includes the next hop mtu in it
>>> so that the sender can fragment the packets.
>> I'm not used to seeing series where different patches apply to different
>> repos!  It took me a while to figure out why "git am" didn't want to
>> work at all (but then I actually looked at the patch).
>>
>> I didn't have a net-next tree handy so my comments are based on reading
>> the patch without applying or building it.
>>
>> There is a special issue with this action, which is a lot like a special
>> issue with the "sample".  That is that the action has flow control that
>> might change the flow in different ways, and this can make an important
>> difference for actions that follow this one.  If one fork of the action
>> pops off a header, and the other one does not, then that makes
>> validating actions that come after it complicated.  I do not see
>> anything here that takes that into account.  I recommend reading the
>> validation code for the sample action for inspiration.
>>
>> In validate_and_copy_check_pkt_len(), it might be better to use
>> nla_policy and nla_parse_nested().  Or maybe not.  I did not look
>> closely.
>>
>> I don't think that execute_check_pkt_len() should need to check for
>> netlink format errors.  The validation code should take care of that.
>> It might also be able to assume a particular order for the attributes,
>> depending on how you implement validation.
>>
>> I'm still not thrilled by the naming.  I don't have any wonderful names
>> though.
>