[ovs-dev] ofproto-dpif-xlate: Fix continuations with associated metering.

Open vSwitch supports the ability to invoke a controller action by way
of a sample action with a specified meter.  In the normal case, this
sample action is transparently generated during xlate processing.  However,
when executing via a continuation, the logic to generate the sample
action when finishing the context freeze was missing.  The result is that
the behavior when action is 'controller(pause,meter_id=1)' does not match
the behavior when action is 'controller(meter_id=1)'.

OVN and other controller solutions may rely on this metering to protect
the control path, so it is critical to preserve metering, whether we are
doing a plain old send to controller, or a continuation.

Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal in "continuations".")
Reported-at: https://issues.redhat.com/browse/FDP-455
Tested-by: Alex Musil <amusil@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 ofproto/ofproto-dpif-xlate.c | 66 +++++++++++++++++++-----------------
 tests/ofproto-dpif.at        | 50 +++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 32 deletions(-)

On 3/7/24 18:25, Aaron Conole wrote:
> Open vSwitch supports the ability to invoke a controller action by way
> of a sample action with a specified meter.  In the normal case, this
> sample action is transparently generated during xlate processing.  However,
> when executing via a continuation, the logic to generate the sample
> action when finishing the context freeze was missing.  The result is that
> the behavior when action is 'controller(pause,meter_id=1)' does not match
> the behavior when action is 'controller(meter_id=1)'.
> 
> OVN and other controller solutions may rely on this metering to protect
> the control path, so it is critical to preserve metering, whether we are
> doing a plain old send to controller, or a continuation.
> 
> Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal in "continuations".")
> Reported-at: https://issues.redhat.com/browse/FDP-455
> Tested-by: Alex Musil <amusil@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  ofproto/ofproto-dpif-xlate.c | 66 +++++++++++++++++++-----------------
>  tests/ofproto-dpif.at        | 50 +++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+), 32 deletions(-)

Thanks, Aaron!  The change seems corrct to me.
See a couple of comments inline.

Best regrads, Ilya Maximets.

> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 89f183182e..4f1e57e6d1 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5080,10 +5080,37 @@ put_controller_user_action(struct xlate_ctx *ctx,
>                             bool dont_send, bool continuation,
>                             uint32_t recirc_id, int len,
>                             enum ofp_packet_in_reason reason,
> +                           uint32_t provider_meter_id,
>                             uint16_t controller_id)
>  {
>      struct user_action_cookie cookie;
>  
> +    /* If the controller action didn't request a meter (indicated by a
> +     * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
> +     * configured through the "controller" virtual meter.
> +     *
> +     * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
> +     * configured. */
> +    uint32_t meter_id;
> +    if (provider_meter_id == UINT32_MAX) {
> +        meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
> +    } else {
> +        meter_id = provider_meter_id;
> +    }
> +
> +    size_t offset;
> +    size_t ac_offset;
> +    if (meter_id != UINT32_MAX) {
> +        /* If controller meter is configured, generate clone(meter,
> +         * userspace) action. */

Might be better to keep the comment the way it were originally.
I find it harder to read the action split in two lines.

> +        offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE);
> +        nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
> +                       UINT32_MAX);
> +        ac_offset = nl_msg_start_nested(ctx->odp_actions,
> +                                        OVS_SAMPLE_ATTR_ACTIONS);
> +        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
> +    }
> +
>      memset(&cookie, 0, sizeof cookie);
>      cookie.type = USER_ACTION_COOKIE_CONTROLLER;
>      cookie.ofp_in_port = OFPP_NONE,
> @@ -5101,6 +5128,11 @@ put_controller_user_action(struct xlate_ctx *ctx,
>      uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
>      odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE,
>                               false, ctx->odp_actions, NULL);
> +
> +    if (meter_id != UINT32_MAX) {
> +        nl_msg_end_nested(ctx->odp_actions, ac_offset);
> +        nl_msg_end_nested(ctx->odp_actions, offset);
> +    }
>  }
>  
>  static void
> @@ -5145,32 +5177,6 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
>      }
>      recirc_refs_add(&ctx->xout->recircs, recirc_id);
>  
> -    /* If the controller action didn't request a meter (indicated by a
> -     * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
> -     * configured through the "controller" virtual meter.
> -     *
> -     * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
> -     * configured. */
> -    uint32_t meter_id;
> -    if (provider_meter_id == UINT32_MAX) {
> -        meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
> -    } else {
> -        meter_id = provider_meter_id;
> -    }
> -
> -    size_t offset;
> -    size_t ac_offset;
> -    if (meter_id != UINT32_MAX) {
> -        /* If controller meter is configured, generate clone(meter, userspace)
> -         * action. */
> -        offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE);
> -        nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
> -                       UINT32_MAX);
> -        ac_offset = nl_msg_start_nested(ctx->odp_actions,
> -                                        OVS_SAMPLE_ATTR_ACTIONS);
> -        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
> -    }
> -
>      /* Generate the datapath flows even if we don't send the packet-in
>       * so that debugging more closely represents normal state. */
>      bool dont_send = false;
> @@ -5178,12 +5184,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
>          dont_send = true;
>      }
>      put_controller_user_action(ctx, dont_send, false, recirc_id, len,
> -                               reason, controller_id);
> -
> -    if (meter_id != UINT32_MAX) {
> -        nl_msg_end_nested(ctx->odp_actions, ac_offset);
> -        nl_msg_end_nested(ctx->odp_actions, offset);
> -    }
> +                               reason, provider_meter_id, controller_id);
>  }
>  
>  /* Creates a frozen state, and allocates a unique recirc id for the given
> @@ -5235,6 +5236,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
>          put_controller_user_action(ctx, false, true, recirc_id,
>                                     ctx->pause->max_len,
>                                     ctx->pause->reason,
> +                                   ctx->pause->provider_meter_id,
>                                     ctx->pause->controller_id);
>      } else {
>          if (ctx->recirc_update_dp_hash) {
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index a1393f7f8e..db205a5316 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6040,6 +6040,7 @@ m4_define([CHECK_CONTINUATION], [dnl
>    ])
>  ])
>  
> +
>  # Check that pause at the end of the pipeline works OK.
>  #
>  # (xlate_continuation() has a special case for no-op actions; this
> @@ -6160,6 +6161,7 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +
>  AT_SETUP([ofproto-dpif - continuation with conntrack])
>  AT_KEYWORDS([continuations pause resume])
>  OVS_VSWITCHD_START


Unrelated changes.

> @@ -6195,6 +6197,54 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([ofproto-dpif - continuation with meters])
> +AT_KEYWORDS([continuations pause meters])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +dnl Add meter with id=1

A period at the end.

> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop rate=1'])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0 dl_dst=50:54:00:00:00:0a actions=goto_table(1)
> +table=1 dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1)
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt])
> +
> +AT_CAPTURE_FILE([ofctl_monitor.log])
> +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log])

May better to wrap the line before the '--detach'.
Modern autotools handle '\' just fine.

And we need to register an on_exit hook to kill this daemon
if something goes wrong.

> +
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'

AT_CHECK.
And maybe also wrap after the port.

> +
> +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])

$() is preffered over ``.

> +OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
> +AT_CHECK([cat ofctl_monitor.log], [0], [dnl
> +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered)
> +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
> +])
> +
> +AT_CHECK([ovs-appctl revalidator/purge], [0])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl
> + n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=goto_table:1
> + table=1, n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1)
> +OFPST_FLOW reply (OF1.3):
> +])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip | sort], [0], [dnl
> +OFPST_METER_CONFIG reply (OF1.3):
> +meter=1 pktps bands=
> +type=drop rate=1
> +])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl
> +OFPST_METER reply (OF1.3) (xid=0x2):
> +meter:1 flow_count:0 packet_in_count:1 byte_in_count:14 duration:0.0s bands:
> +0: packet_count:0 byte_count:0
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto-dpif - continuation with patch port])
>  AT_KEYWORDS([continuations pause resume])
>  OVS_VSWITCHD_START(

Ilya Maximets <i.maximets@ovn.org> writes:

> On 3/7/24 18:25, Aaron Conole wrote:
>> Open vSwitch supports the ability to invoke a controller action by way
>> of a sample action with a specified meter.  In the normal case, this
>> sample action is transparently generated during xlate processing.  However,
>> when executing via a continuation, the logic to generate the sample
>> action when finishing the context freeze was missing.  The result is that
>> the behavior when action is 'controller(pause,meter_id=1)' does not match
>> the behavior when action is 'controller(meter_id=1)'.
>> 
>> OVN and other controller solutions may rely on this metering to protect
>> the control path, so it is critical to preserve metering, whether we are
>> doing a plain old send to controller, or a continuation.
>> 
>> Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal in "continuations".")
>> Reported-at: https://issues.redhat.com/browse/FDP-455
>> Tested-by: Alex Musil <amusil@redhat.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  ofproto/ofproto-dpif-xlate.c | 66 +++++++++++++++++++-----------------
>>  tests/ofproto-dpif.at        | 50 +++++++++++++++++++++++++++
>>  2 files changed, 84 insertions(+), 32 deletions(-)
>
> Thanks, Aaron!  The change seems corrct to me.
> See a couple of comments inline.
>
> Best regrads, Ilya Maximets.
>
>> 
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 89f183182e..4f1e57e6d1 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -5080,10 +5080,37 @@ put_controller_user_action(struct xlate_ctx *ctx,
>>                             bool dont_send, bool continuation,
>>                             uint32_t recirc_id, int len,
>>                             enum ofp_packet_in_reason reason,
>> +                           uint32_t provider_meter_id,
>>                             uint16_t controller_id)
>>  {
>>      struct user_action_cookie cookie;
>>  
>> +    /* If the controller action didn't request a meter (indicated by a
>> +     * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
>> +     * configured through the "controller" virtual meter.
>> +     *
>> +     * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
>> +     * configured. */
>> +    uint32_t meter_id;
>> +    if (provider_meter_id == UINT32_MAX) {
>> +        meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
>> +    } else {
>> +        meter_id = provider_meter_id;
>> +    }
>> +
>> +    size_t offset;
>> +    size_t ac_offset;
>> +    if (meter_id != UINT32_MAX) {
>> +        /* If controller meter is configured, generate clone(meter,
>> +         * userspace) action. */
>
> Might be better to keep the comment the way it were originally.
> I find it harder to read the action split in two lines.

Okay - I will put it back.  I think I split it because of the line
lengths, but maybe it would be better to put clone(meter,userspace) on
the second line.

>> +        offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE);
>> +        nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
>> +                       UINT32_MAX);
>> +        ac_offset = nl_msg_start_nested(ctx->odp_actions,
>> +                                        OVS_SAMPLE_ATTR_ACTIONS);
>> +        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
>> +    }
>> +
>>      memset(&cookie, 0, sizeof cookie);
>>      cookie.type = USER_ACTION_COOKIE_CONTROLLER;
>>      cookie.ofp_in_port = OFPP_NONE,
>> @@ -5101,6 +5128,11 @@ put_controller_user_action(struct xlate_ctx *ctx,
>>      uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
>>      odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE,
>>                               false, ctx->odp_actions, NULL);
>> +
>> +    if (meter_id != UINT32_MAX) {
>> +        nl_msg_end_nested(ctx->odp_actions, ac_offset);
>> +        nl_msg_end_nested(ctx->odp_actions, offset);
>> +    }
>>  }
>>  
>>  static void
>> @@ -5145,32 +5177,6 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
>>      }
>>      recirc_refs_add(&ctx->xout->recircs, recirc_id);
>>  
>> -    /* If the controller action didn't request a meter (indicated by a
>> -     * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
>> -     * configured through the "controller" virtual meter.
>> -     *
>> -     * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
>> -     * configured. */
>> -    uint32_t meter_id;
>> -    if (provider_meter_id == UINT32_MAX) {
>> -        meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
>> -    } else {
>> -        meter_id = provider_meter_id;
>> -    }
>> -
>> -    size_t offset;
>> -    size_t ac_offset;
>> -    if (meter_id != UINT32_MAX) {
>> -        /* If controller meter is configured, generate clone(meter, userspace)
>> -         * action. */
>> -        offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE);
>> -        nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
>> -                       UINT32_MAX);
>> -        ac_offset = nl_msg_start_nested(ctx->odp_actions,
>> -                                        OVS_SAMPLE_ATTR_ACTIONS);
>> -        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
>> -    }
>> -
>>      /* Generate the datapath flows even if we don't send the packet-in
>>       * so that debugging more closely represents normal state. */
>>      bool dont_send = false;
>> @@ -5178,12 +5184,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
>>          dont_send = true;
>>      }
>>      put_controller_user_action(ctx, dont_send, false, recirc_id, len,
>> -                               reason, controller_id);
>> -
>> -    if (meter_id != UINT32_MAX) {
>> -        nl_msg_end_nested(ctx->odp_actions, ac_offset);
>> -        nl_msg_end_nested(ctx->odp_actions, offset);
>> -    }
>> +                               reason, provider_meter_id, controller_id);
>>  }
>>  
>>  /* Creates a frozen state, and allocates a unique recirc id for the given
>> @@ -5235,6 +5236,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
>>          put_controller_user_action(ctx, false, true, recirc_id,
>>                                     ctx->pause->max_len,
>>                                     ctx->pause->reason,
>> +                                   ctx->pause->provider_meter_id,
>>                                     ctx->pause->controller_id);
>>      } else {
>>          if (ctx->recirc_update_dp_hash) {
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index a1393f7f8e..db205a5316 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -6040,6 +6040,7 @@ m4_define([CHECK_CONTINUATION], [dnl
>>    ])
>>  ])
>>  
>> +
>>  # Check that pause at the end of the pipeline works OK.
>>  #
>>  # (xlate_continuation() has a special case for no-op actions; this
>> @@ -6160,6 +6161,7 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc
>>  OVS_VSWITCHD_STOP
>>  AT_CLEANUP
>>  
>> +
>>  AT_SETUP([ofproto-dpif - continuation with conntrack])
>>  AT_KEYWORDS([continuations pause resume])
>>  OVS_VSWITCHD_START
>
>
> Unrelated changes.

Yuck... I don't know why that got in.  Will trim.

>> @@ -6195,6 +6197,54 @@ AT_CHECK([test 1 = `$PYTHON3 "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc
>>  OVS_VSWITCHD_STOP
>>  AT_CLEANUP
>>  
>> +AT_SETUP([ofproto-dpif - continuation with meters])
>> +AT_KEYWORDS([continuations pause meters])
>> +OVS_VSWITCHD_START
>> +add_of_ports br0 1 2
>> +
>> +dnl Add meter with id=1
>
> A period at the end.

Ack

>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop rate=1'])
>> +
>> +AT_DATA([flows.txt], [dnl
>> +table=0 dl_dst=50:54:00:00:00:0a actions=goto_table(1)
>> +table=1 dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1)
>> +])
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt])
>> +
>> +AT_CAPTURE_FILE([ofctl_monitor.log])
>> +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log])
>
> May better to wrap the line before the '--detach'.
> Modern autotools handle '\' just fine.
>
> And we need to register an on_exit hook to kill this daemon
> if something goes wrong.

Ack to both

>> +
>> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
>
> AT_CHECK.
> And maybe also wrap after the port.
>
>> +
>> +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
>
> $() is preffered over ``.

Okay - I think I copied these from another test in the file, so I stuck
with the original syntax.  I can change it.

>> +OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
>> +AT_CHECK([cat ofctl_monitor.log], [0], [dnl
>> +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered)
>> +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234
>> +])
>> +
>> +AT_CHECK([ovs-appctl revalidator/purge], [0])
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl
>> + n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=goto_table:1
>> + table=1, n_packets=1, n_bytes=14, dl_dst=50:54:00:00:00:0a actions=controller(pause,meter_id=1)
>> +OFPST_FLOW reply (OF1.3):
>> +])
>> +
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip | sort], [0], [dnl
>> +OFPST_METER_CONFIG reply (OF1.3):
>> +meter=1 pktps bands=
>> +type=drop rate=1
>> +])
>> +
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl
>> +OFPST_METER reply (OF1.3) (xid=0x2):
>> +meter:1 flow_count:0 packet_in_count:1 byte_in_count:14 duration:0.0s bands:
>> +0: packet_count:0 byte_count:0
>> +])
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>>  AT_SETUP([ofproto-dpif - continuation with patch port])
>>  AT_KEYWORDS([continuations pause resume])
>>  OVS_VSWITCHD_START(