| Message ID | 1526383418-26551-1-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Free packets on TUNNEL_PUSH if may_steal. | expand |
On Tue, May 15, 2018 at 02:23:38PM +0300, Ilya Maximets wrote: > Unconditional return may cause packet leak in case of > 'may_steal == true'. > > Additionally, removed redundant checking for depth level and > clarified ignoring of the 'false' value of 'may_steal'. > > CC: Sugesh Chandran <sugesh.chandran@intel.com> > Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by > combining recirc actions at xlate.") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> Thanks. This seems reasonable to me. Did you take a look at the other cases in the function to see whether they have the same problem? Since this is in dpif-netdev I'll leave the final review to Ian for his branch.
On 15.05.2018 20:18, Ben Pfaff wrote: > On Tue, May 15, 2018 at 02:23:38PM +0300, Ilya Maximets wrote: >> Unconditional return may cause packet leak in case of >> 'may_steal == true'. >> >> Additionally, removed redundant checking for depth level and >> clarified ignoring of the 'false' value of 'may_steal'. >> >> CC: Sugesh Chandran <sugesh.chandran@intel.com> >> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by >> combining recirc actions at xlate.") >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > Thanks. This seems reasonable to me. > > Did you take a look at the other cases in the function to see whether > they have the same problem? I reviewed other cases inside 'dp_execute_cb()' and they seems correct, at least in terms of freeing the packets. --- It's not related to current patch, but one thing that I found is that OVS_ACTION_ATTR_CT unconditionally modifies the packets regardless of 'may_steal' value. For example it could update ip addresses for NAT purposes inside 'conntrack_execute()'. Is it acceptable? Are we always in kind of cloned state while excuting CT? I think this should be fixed or clarified in code by comments, because this breaks the API of 'dp_execute_action()'. Best regards, Ilya Maximets.
On Wed, May 16, 2018 at 6:01 AM, Ilya Maximets <i.maximets@samsung.com> wrote: > On 15.05.2018 20:18, Ben Pfaff wrote: > > On Tue, May 15, 2018 at 02:23:38PM +0300, Ilya Maximets wrote: > >> Unconditional return may cause packet leak in case of > >> 'may_steal == true'. > >> > >> Additionally, removed redundant checking for depth level and > >> clarified ignoring of the 'false' value of 'may_steal'. > >> > >> CC: Sugesh Chandran <sugesh.chandran@intel.com> > >> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by > >> combining recirc actions at xlate.") > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > > Thanks. This seems reasonable to me. > > > > Did you take a look at the other cases in the function to see whether > > they have the same problem? > > I reviewed other cases inside 'dp_execute_cb()' and they seems correct, > at least in terms of freeing the packets. > > --- > It's not related to current patch, but one thing that I found is that > OVS_ACTION_ATTR_CT unconditionally modifies the packets regardless of > 'may_steal' value. For example it could update ip addresses for NAT > purposes inside 'conntrack_execute()'. > Is it acceptable? Are we always in kind of cloned state while excuting CT? > I think this should be fixed or clarified in code by comments, because > this breaks the API of 'dp_execute_action()'. > Thanks for pointing that out Ilya. I'll propose something to address it. Darrell > > Best regards, Ilya Maximets. >
On 15.05.2018 20:18, Ben Pfaff wrote: > On Tue, May 15, 2018 at 02:23:38PM +0300, Ilya Maximets wrote: >> Unconditional return may cause packet leak in case of >> 'may_steal == true'. >> >> Additionally, removed redundant checking for depth level and >> clarified ignoring of the 'false' value of 'may_steal'. >> >> CC: Sugesh Chandran <sugesh.chandran@intel.com> >> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by >> combining recirc actions at xlate.") >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > Thanks. This seems reasonable to me. > > Did you take a look at the other cases in the function to see whether > they have the same problem? > > Since this is in dpif-netdev I'll leave the final review to Ian for his > branch. Hello, Ian. What do you think about this patch? Sorry for pinging, but this is a really bad issue which drains the mempool in case of badly configured OpenFlow rules. We're faced this on one of SDN setups in our testing lab. Is it possible to include the fix in one of the next pull requests? Best regards, Ilya Maximets.
On 24.05.2018 11:52, Ilya Maximets wrote: > On 15.05.2018 20:18, Ben Pfaff wrote: >> On Tue, May 15, 2018 at 02:23:38PM +0300, Ilya Maximets wrote: >>> Unconditional return may cause packet leak in case of >>> 'may_steal == true'. >>> >>> Additionally, removed redundant checking for depth level and >>> clarified ignoring of the 'false' value of 'may_steal'. >>> >>> CC: Sugesh Chandran <sugesh.chandran@intel.com> >>> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by >>> combining recirc actions at xlate.") >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> >> Thanks. This seems reasonable to me. >> >> Did you take a look at the other cases in the function to see whether >> they have the same problem? >> >> Since this is in dpif-netdev I'll leave the final review to Ian for his >> branch. > > Hello, Ian. > What do you think about this patch? > Sorry for pinging, but this is a really bad issue which drains the mempool > in case of badly configured OpenFlow rules. We're faced this on one of SDN > setups in our testing lab. Is it possible to include the fix in one of the > next pull requests? I've sent v2 rebased on top of current master with updates related to recent patches from Darrell. v1 should be used for stable branches: 2.8 and 2.9. > > Best regards, Ilya Maximets. >
> On 24.05.2018 11:52, Ilya Maximets wrote: > > On 15.05.2018 20:18, Ben Pfaff wrote: > >> On Tue, May 15, 2018 at 02:23:38PM +0300, Ilya Maximets wrote: > >>> Unconditional return may cause packet leak in case of 'may_steal == > >>> true'. > >>> > >>> Additionally, removed redundant checking for depth level and > >>> clarified ignoring of the 'false' value of 'may_steal'. > >>> > >>> CC: Sugesh Chandran <sugesh.chandran@intel.com> > >>> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by > >>> combining recirc actions at xlate.") > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >> > >> Thanks. This seems reasonable to me. > >> > >> Did you take a look at the other cases in the function to see whether > >> they have the same problem? > >> > >> Since this is in dpif-netdev I'll leave the final review to Ian for > >> his branch. > > > > Hello, Ian. > > What do you think about this patch? > > Sorry for pinging, but this is a really bad issue which drains the > > mempool in case of badly configured OpenFlow rules. We're faced this > > on one of SDN setups in our testing lab. Is it possible to include the > > fix in one of the next pull requests? > > I've sent v2 rebased on top of current master with updates related to > recent patches from Darrell. > v1 should be used for stable branches: 2.8 and 2.9. > Thanks Ilya, I'll look at this now, we'll be able to include it in the new releases for branch 2.8 and 2.9 also this week. Ian > > > > Best regards, Ilya Maximets. > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index f86ed2a..294cd87 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -5672,12 +5672,23 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, break; case OVS_ACTION_ATTR_TUNNEL_PUSH: - if (*depth < MAX_RECIRC_DEPTH) { - dp_packet_batch_apply_cutlen(packets_); - push_tnl_action(pmd, a, packets_); - return; + /* + * XXX: 'may_steal' concept is broken here, because we're + * unconditionally changing the packets just like for other PUSH_* + * actions in 'odp_execute()'. 'false' value could be ignored, + * because we could reach here only after clone, but we still need + * to free the packets in case 'may_steal == true'. + */ + if (may_steal) { + /* We're requested to push tunnel header, but also we need to take + * the ownership of these packets. Thus, we can avoid performing + * the action, because the caller will not use the result anyway. + * Just break to free the batch. */ + break; } - break; + dp_packet_batch_apply_cutlen(packets_); + push_tnl_action(pmd, a, packets_); + return; case OVS_ACTION_ATTR_TUNNEL_POP: if (*depth < MAX_RECIRC_DEPTH) {
Unconditional return may cause packet leak in case of 'may_steal == true'. Additionally, removed redundant checking for depth level and clarified ignoring of the 'false' value of 'may_steal'. CC: Sugesh Chandran <sugesh.chandran@intel.com> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc actions at xlate.") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/dpif-netdev.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)