| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 18 Jun 2024 11:47:36 +0200
From: Ilya Maximets <i.maximets@....org>
To: Adrián Moreno <amorenoz@...hat.com>
Cc: i.maximets@....org, netdev@...r.kernel.org, aconole@...hat.com,
echaudro@...hat.com, horms@...nel.org, dev@...nvswitch.org,
Donald Hunter <donald.hunter@...il.com>, Jakub Kicinski <kuba@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Pravin B Shelar <pshelar@....org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action
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 <amorenoz@...hat.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.
Best regards, Ilya Maximets.
Powered by blists - more mailing lists