Hi Ben and Greg, Please see one comment below inline
Thanks Numan On Tue, Feb 12, 2019 at 7:57 AM Ben Pfaff <[email protected]> wrote: > Greg, I recommend that you take a look at this one. > > (More commentary below.) > > On Thu, Jan 10, 2019 at 11:29:48PM +0530, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > [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. > > >From what I see from the sample code, it checks if the actions inside the sample modify the flow key or not and set the 'sample_arg->exec' accordingly if the 'sample' action is not the last action. In the ovs-vswitchd patch (patch 2 of this series) when it translates the 'check_pkt_len' action, 'check_pkt_len' will be the last action of the flow. In the datapath implementation I am thinking to mandate 'check_pkt_len' to be last action. If any actions follow 'check_pkt_len', then entire flow should be rejected. Any thoughts, comments on this. I really don't see a scenario where another action would follow 'check_pkt_len'. Thanks Numan > 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. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
