On 6/18/24 11:47, Ilya Maximets wrote:
> On 6/18/24 09:33, Adrián Moreno wrote:
>> On Mon, Jun 17, 2024 at 12:44:45PM GMT, Ilya Maximets wrote:
>>> On 6/3/24 20:56, Adrian Moreno wrote:
>>>> Add support for a new action: emit_sample.
>>>>
>>>> This action accepts a u32 group id and a variable-length cookie and uses
>>>> the psample multicast group to make the packet available for
>>>> observability.
>>>>
>>>> The maximum length of the user-defined cookie is set to 16, same as
>>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>>>
>>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>>>> ---
>>>> Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
>>>> include/uapi/linux/openvswitch.h | 25 ++++++++++++
>>>> net/openvswitch/actions.c | 50 +++++++++++++++++++++++
>>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++-
>>>> 4 files changed, 124 insertions(+), 1 deletion(-)
>>>
>>> Some nits below, beside ones already mentioned.
>>>
>>
>> Thanks, Ilya.
>>
>>>>
>>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
>>>> b/Documentation/netlink/specs/ovs_flow.yaml
>>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>>> @@ -727,6 +727,12 @@ attribute-sets:
>>>> name: dec-ttl
>>>> type: nest
>>>> nested-attributes: dec-ttl-attrs
>>>> + -
>>>> + name: emit-sample
>>>> + type: nest
>>>> + nested-attributes: emit-sample-attrs
>>>> + doc: |
>>>> + Sends a packet sample to psample for external observation.
>>>> -
>>>> name: tunnel-key-attrs
>>>> enum-name: ovs-tunnel-key-attr
>>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>> -
>>>> name: gbp
>>>> type: u32
>>>> + -
>>>> + name: emit-sample-attrs
>>>> + enum-name: ovs-emit-sample-attr
>>>> + name-prefix: ovs-emit-sample-attr-
>>>> + attributes:
>>>> + -
>>>> + name: group
>>>> + type: u32
>>>> + -
>>>> + name: cookie
>>>> + type: binary
>>>>
>>>> operations:
>>>> name-prefix: ovs-flow-cmd-
>>>> diff --git a/include/uapi/linux/openvswitch.h
>>>> b/include/uapi/linux/openvswitch.h
>>>> index efc82c318fa2..a0e9dde0584a 100644
>>>> --- a/include/uapi/linux/openvswitch.h
>>>> +++ b/include/uapi/linux/openvswitch.h
>>>> @@ -914,6 +914,30 @@ struct check_pkt_len_arg {
>>>> };
>>>> #endif
>>>>
>>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>>>> +/**
>>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>>>> + * action.
>>>> + *
>>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of
>>>> the
>>>> + * sample.
>>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
>>>> contains
>>>> + * user-defined metadata. The maximum length is 16 bytes.
>>>
>>> s/16/OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE/
>>>
>>>> + *
>>>> + * Sends the packet to the psample multicast group with the specified
>>>> group and
>>>> + * cookie. It is possible to combine this action with the
>>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being
>>>> emitted.
>>>> + */
>>>> +enum ovs_emit_sample_attr {
>>>> + OVS_EMIT_SAMPLE_ATTR_UNPSEC,
>>>> + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
>>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */
>>>> + __OVS_EMIT_SAMPLE_ATTR_MAX
>>>> +};
>>>> +
>>>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
>>>> +
>>>> +
>>>> /**
>>>> * enum ovs_action_attr - Action types.
>>>> *
>>>> @@ -1004,6 +1028,7 @@ enum ovs_action_attr {
>>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>>>> OVS_ACTION_ATTR_DROP, /* u32 error code. */
>>>> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>>>>
>>>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
>>>> * from userspace. */
>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>> index 964225580824..3b4dba0ded59 100644
>>>> --- a/net/openvswitch/actions.c
>>>> +++ b/net/openvswitch/actions.c
>>>> @@ -24,6 +24,11 @@
>>>> #include <net/checksum.h>
>>>> #include <net/dsfield.h>
>>>> #include <net/mpls.h>
>>>> +
>>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
>>>> +#include <net/psample.h>
>>>> +#endif
>>>> +
>>>> #include <net/sctp/checksum.h>
>>>>
>>>> #include "datapath.h"
>>>> @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb,
>>>> struct sw_flow_key *key)
>>>> return 0;
>>>> }
>>>>
>>>> +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb,
>>>> + const struct sw_flow_key *key,
>>>> + const struct nlattr *attr)
>>>> +{
>>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
>>>> + struct psample_group psample_group = {};
>>>> + struct psample_metadata md = {};
>>>> + struct vport *input_vport;
>>>> + const struct nlattr *a;
>>>> + int rem;
>>>> +
>>>> + for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
>>>> + a = nla_next(a, &rem)) {
>>>
>>> Since the action is strictly validated, can use use nla_for_each_attr()
>>> or nla_for_each_nested() ?
>>>
>>
>> Probably, yes.
>>
>>>> + switch (nla_type(a)) {
>>>> + case OVS_EMIT_SAMPLE_ATTR_GROUP:
>>>> + psample_group.group_num = nla_get_u32(a);
>>>> + break;
>>>> +
>>>> + case OVS_EMIT_SAMPLE_ATTR_COOKIE:
>>>> + md.user_cookie = nla_data(a);
>>>> + md.user_cookie_len = nla_len(a);
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + psample_group.net = ovs_dp_get_net(dp);
>>>> +
>>>> + input_vport = ovs_vport_rcu(dp, key->phy.in_port);
>>>> + if (!input_vport)
>>>> + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL);
>>>
>>> We may need to check that we actually found the local port.
>>>
>>
>> Sure. What can cause the local port not to exist?
>
> I would assume that since we're only protected by RCU here, there can be
> a race with datapath destruction that will remove the local port.
But, actually, we don't even need to look anything up. The original
input vport should be available in OVS_CB(skb)->input_vport.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
