[ovs-dev,net,1/2] net: openvswitch: limit the number of recursions from action sets

The ovs module allows for some actions to recursively contain an action
list for complex scenarios, such as sampling, checking lengths, etc.
When these actions are copied into the internal flow table, they are
evaluated to validate that such actions make sense, and these calls
happen recursively.

The ovs-vswitchd userspace won't emit more than 16 recursion levels
deep.  However, the module has no such limit and will happily accept
limits larger than 16 levels nested.  Prevent this by tracking the
number of recursions happening and manually limiting it to 16 levels
nested.

The initial implementation of the sample action would track this depth
and prevent more than 3 levels of recursion, but this was removed to
support the clone use case, rather than limited at the current userspace
limit.

Fixes: 798c166173ff ("openvswitch: Optimize sample action for the clone use cases")
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 net/openvswitch/flow_netlink.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

On Tue, Feb 6, 2024 at 2:11 PM Aaron Conole <aconole@redhat.com> wrote:
>
> The ovs module allows for some actions to recursively contain an action
> list for complex scenarios, such as sampling, checking lengths, etc.
> When these actions are copied into the internal flow table, they are
> evaluated to validate that such actions make sense, and these calls
> happen recursively.
>
> The ovs-vswitchd userspace won't emit more than 16 recursion levels
> deep.  However, the module has no such limit and will happily accept
> limits larger than 16 levels nested.  Prevent this by tracking the
> number of recursions happening and manually limiting it to 16 levels
> nested.
>
> The initial implementation of the sample action would track this depth
> and prevent more than 3 levels of recursion, but this was removed to
> support the clone use case, rather than limited at the current userspace
> limit.
>
> Fixes: 798c166173ff ("openvswitch: Optimize sample action for the clone use cases")
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  net/openvswitch/flow_netlink.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 88965e2068ac..ba5cfa67a720 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -48,6 +48,9 @@ struct ovs_len_tbl {
>
>  #define OVS_ATTR_NESTED -1
>  #define OVS_ATTR_VARIABLE -2
> +#define OVS_COPY_ACTIONS_MAX_DEPTH 16
> +
> +static DEFINE_PER_CPU(int, copy_actions_depth);
>
>  static bool actions_may_change_flow(const struct nlattr *actions)
>  {
> @@ -3148,11 +3151,11 @@ static int copy_action(const struct nlattr *from,
>         return 0;
>  }
>
> -static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> -                                 const struct sw_flow_key *key,
> -                                 struct sw_flow_actions **sfa,
> -                                 __be16 eth_type, __be16 vlan_tci,
> -                                 u32 mpls_label_count, bool log)
> +static int ___ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> +                                  const struct sw_flow_key *key,
> +                                  struct sw_flow_actions **sfa,
> +                                  __be16 eth_type, __be16 vlan_tci,
> +                                  u32 mpls_label_count, bool log)
>  {
>         u8 mac_proto = ovs_key_mac_proto(key);
>         const struct nlattr *a;
> @@ -3478,6 +3481,26 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>         return 0;
>  }
>
> +static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> +                                 const struct sw_flow_key *key,
> +                                 struct sw_flow_actions **sfa,
> +                                 __be16 eth_type, __be16 vlan_tci,
> +                                 u32 mpls_label_count, bool log)
> +{
> +       int level = this_cpu_read(copy_actions_depth);
> +       int ret;
> +
> +       if (level > OVS_COPY_ACTIONS_MAX_DEPTH)
> +               return -EOVERFLOW;
> +

This code seems to run in process context.

Using per cpu limit would not work, unless you disabled migration ?

> +       __this_cpu_inc(copy_actions_depth);
> +       ret = ___ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
> +                                     vlan_tci, mpls_label_count, log);
> +       __this_cpu_dec(copy_actions_depth);
> +
> +       return ret;
> +}
> +
>  /* 'key' must be the masked key. */
>  int ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>                          const struct sw_flow_key *key,
> --
> 2.41.0
>

Eric Dumazet <edumazet@google.com> writes:

> On Tue, Feb 6, 2024 at 2:11 PM Aaron Conole <aconole@redhat.com> wrote:
>>
>> The ovs module allows for some actions to recursively contain an action
>> list for complex scenarios, such as sampling, checking lengths, etc.
>> When these actions are copied into the internal flow table, they are
>> evaluated to validate that such actions make sense, and these calls
>> happen recursively.
>>
>> The ovs-vswitchd userspace won't emit more than 16 recursion levels
>> deep.  However, the module has no such limit and will happily accept
>> limits larger than 16 levels nested.  Prevent this by tracking the
>> number of recursions happening and manually limiting it to 16 levels
>> nested.
>>
>> The initial implementation of the sample action would track this depth
>> and prevent more than 3 levels of recursion, but this was removed to
>> support the clone use case, rather than limited at the current userspace
>> limit.
>>
>> Fixes: 798c166173ff ("openvswitch: Optimize sample action for the clone use cases")
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  net/openvswitch/flow_netlink.c | 33 ++++++++++++++++++++++++++++-----
>>  1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index 88965e2068ac..ba5cfa67a720 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -48,6 +48,9 @@ struct ovs_len_tbl {
>>
>>  #define OVS_ATTR_NESTED -1
>>  #define OVS_ATTR_VARIABLE -2
>> +#define OVS_COPY_ACTIONS_MAX_DEPTH 16
>> +
>> +static DEFINE_PER_CPU(int, copy_actions_depth);
>>
>>  static bool actions_may_change_flow(const struct nlattr *actions)
>>  {
>> @@ -3148,11 +3151,11 @@ static int copy_action(const struct nlattr *from,
>>         return 0;
>>  }
>>
>> -static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>> -                                 const struct sw_flow_key *key,
>> -                                 struct sw_flow_actions **sfa,
>> -                                 __be16 eth_type, __be16 vlan_tci,
>> -                                 u32 mpls_label_count, bool log)
>> +static int ___ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>> +                                  const struct sw_flow_key *key,
>> +                                  struct sw_flow_actions **sfa,
>> +                                  __be16 eth_type, __be16 vlan_tci,
>> +                                  u32 mpls_label_count, bool log)
>>  {
>>         u8 mac_proto = ovs_key_mac_proto(key);
>>         const struct nlattr *a;
>> @@ -3478,6 +3481,26 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>>         return 0;
>>  }
>>
>> +static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>> +                                 const struct sw_flow_key *key,
>> +                                 struct sw_flow_actions **sfa,
>> +                                 __be16 eth_type, __be16 vlan_tci,
>> +                                 u32 mpls_label_count, bool log)
>> +{
>> +       int level = this_cpu_read(copy_actions_depth);
>> +       int ret;
>> +
>> +       if (level > OVS_COPY_ACTIONS_MAX_DEPTH)
>> +               return -EOVERFLOW;
>> +
>
> This code seems to run in process context.
>
> Using per cpu limit would not work, unless you disabled migration ?

Oops - I didn't consider it.

Given that, maybe the best approach would not to rely on per-cpu
counter. I'll respin in the next series with a depth counter that I pass
to the function instead and compare that.  I guess that should address
migration and eliminate the need for per-cpu counter.

Does it make sense?

>> +       __this_cpu_inc(copy_actions_depth);
>> +       ret = ___ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
>> +                                     vlan_tci, mpls_label_count, log);
>> +       __this_cpu_dec(copy_actions_depth);
>> +
>> +       return ret;
>> +}
>> +
>>  /* 'key' must be the masked key. */
>>  int ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>>                          const struct sw_flow_key *key,
>> --
>> 2.41.0
>>

On Tue, Feb 6, 2024 at 3:55 PM Aaron Conole <aconole@redhat.com> wrote:
>
>
> Oops - I didn't consider it.
>
> Given that, maybe the best approach would not to rely on per-cpu
> counter. I'll respin in the next series with a depth counter that I pass
> to the function instead and compare that.  I guess that should address
> migration and eliminate the need for per-cpu counter.
>
> Does it make sense?

Sure, a depth parameter would work much better ;)

Eric Dumazet <edumazet@google.com> writes:

> On Tue, Feb 6, 2024 at 3:55 PM Aaron Conole <aconole@redhat.com> wrote:
>>
>>
>> Oops - I didn't consider it.
>>
>> Given that, maybe the best approach would not to rely on per-cpu
>> counter. I'll respin in the next series with a depth counter that I pass
>> to the function instead and compare that.  I guess that should address
>> migration and eliminate the need for per-cpu counter.
>>
>> Does it make sense?
>
> Sure, a depth parameter would work much better ;)

Okay - I'll give time for others to comment and resubmit in ~24 hours
unless there's a reason to submit sooner.