| Message ID | 1484179204-30234-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Jan 11, 2017 at 04:00:04PM -0800, Andy Zhou wrote: > User space implementation of the sample action is not consistent with > kernel datapath. In kernel datapath, the side effects of actions > within the sample actions are not visible to the subsequent actions. > Current user space handling does not follow the same logic. This patch > makes them consistent. > > Signed-off-by: Andy Zhou <azhou@ovn.org> Thanks! This looks like a bug fix, but I suspect that there's no way for existing code to actually trigger the bug, because I think that OVS at least up to 2.6 only ever puts actions that do not modify the packet into a sample action. Acked-by: Ben Pfaff <blp@ovn.org>
On Thu, Feb 2, 2017 at 10:56 AM, Ben Pfaff <blp@ovn.org> wrote: > On Wed, Jan 11, 2017 at 04:00:04PM -0800, Andy Zhou wrote: >> User space implementation of the sample action is not consistent with >> kernel datapath. In kernel datapath, the side effects of actions >> within the sample actions are not visible to the subsequent actions. >> Current user space handling does not follow the same logic. This patch >> makes them consistent. >> >> Signed-off-by: Andy Zhou <azhou@ovn.org> > > Thanks! > > This looks like a bug fix, but I suspect that there's no way for > existing code to actually trigger the bug, because I think that OVS at > least up to 2.6 only ever puts actions that do not modify the packet > into a sample action. > > Acked-by: Ben Pfaff <blp@ovn.org> This patch was sent out before we have decided to add the userspace datapath 'clone' action. Now that we have the clone action. The sample action will continue to be used for the current use case, so your comment will continue to apply. The fix adds an extra packet copy. The current use cases are not performance critical so may it is not a big deal. In case this becomes an issue, we then should revisit the logic here an see if we can avoid packet copy in case it is not required. Pushed to master. Thanks for the review and comment.
diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 73e1016..cec7432 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -523,8 +523,16 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, } } + if (!steal) { + /* The 'subactions' may modify the packet, but the modification + * should not propagate beyond this sample action. Make a copy + * the packet in case we don't own the packet, so that the + * 'subactions' are only applid to the clone. 'odp_execute_actions' + * will free the clone. */ + packet = dp_packet_clone(packet); + } packet_batch_init_packet(&pb, packet); - odp_execute_actions(dp, &pb, steal, nl_attr_get(subactions), + odp_execute_actions(dp, &pb, true, nl_attr_get(subactions), nl_attr_get_size(subactions), dp_execute_action); }
User space implementation of the sample action is not consistent with kernel datapath. In kernel datapath, the side effects of actions within the sample actions are not visible to the subsequent actions. Current user space handling does not follow the same logic. This patch makes them consistent. Signed-off-by: Andy Zhou <azhou@ovn.org> --- lib/odp-execute.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)