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

Ben Pfaff blp at ovn.org
Tue Feb 12 02:27:00 UTC 2019


Greg, I recommend that you take a look at this one.

(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.


More information about the dev mailing list