| 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: Wed, 15 Mar 2023 07:19:01 +0100
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: edward.cree@....com
Cc: linux-net-drivers@....com, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, edumazet@...gle.com,
Edward Cree <ecree.xilinx@...il.com>, netdev@...r.kernel.org,
habetsm.xilinx@...il.com
Subject: Re: [PATCH net-next 1/5] sfc: add notion of match on enc keys to MAE
machinery
On Tue, Mar 14, 2023 at 05:35:21PM +0000, edward.cree@....com wrote:
> From: Edward Cree <ecree.xilinx@...il.com>
>
> Extend the MAE caps check to validate that the hardware supports used
> outer-header matches.
> Extend efx_mae_populate_match_criteria() to fill in the outer rule ID
> and VNI match fields.
> Nothing yet populates these match fields, nor creates outer rules.
>
> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
> ---
> drivers/net/ethernet/sfc/mae.c | 104 ++++++++++++++++++++++++++++++++-
> drivers/net/ethernet/sfc/mae.h | 3 +
> drivers/net/ethernet/sfc/tc.h | 24 ++++++++
> 3 files changed, 129 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
> index c53d354c1fb2..1a285facda34 100644
> --- a/drivers/net/ethernet/sfc/mae.c
> +++ b/drivers/net/ethernet/sfc/mae.c
> @@ -254,13 +254,23 @@ static int efx_mae_get_rule_fields(struct efx_nic *efx, u32 cmd,
> size_t outlen;
> int rc, i;
>
> + /* AR and OR caps MCDIs have identical layout, so we are using the
> + * same code for both.
> + */
> + BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_LEN(MAE_NUM_FIELDS) <
> + MC_CMD_MAE_GET_OR_CAPS_OUT_LEN(MAE_NUM_FIELDS));
> BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_IN_LEN);
> + BUILD_BUG_ON(MC_CMD_MAE_GET_OR_CAPS_IN_LEN);
>
> rc = efx_mcdi_rpc(efx, cmd, NULL, 0, outbuf, sizeof(outbuf), &outlen);
> if (rc)
> return rc;
> + BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_COUNT_OFST !=
> + MC_CMD_MAE_GET_OR_CAPS_OUT_COUNT_OFST);
> count = MCDI_DWORD(outbuf, MAE_GET_AR_CAPS_OUT_COUNT);
> memset(field_support, MAE_FIELD_UNSUPPORTED, MAE_NUM_FIELDS);
> + BUILD_BUG_ON(MC_CMD_MAE_GET_AR_CAPS_OUT_FIELD_FLAGS_OFST !=
> + MC_CMD_MAE_GET_OR_CAPS_OUT_FIELD_FLAGS_OFST);
> caps = _MCDI_DWORD(outbuf, MAE_GET_AR_CAPS_OUT_FIELD_FLAGS);
> /* We're only interested in the support status enum, not any other
> * flags, so just extract that from each entry.
> @@ -278,8 +288,12 @@ int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps)
> rc = efx_mae_get_basic_caps(efx, caps);
> if (rc)
> return rc;
> - return efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_AR_CAPS,
> - caps->action_rule_fields);
> + rc = efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_AR_CAPS,
> + caps->action_rule_fields);
> + if (rc)
> + return rc;
> + return efx_mae_get_rule_fields(efx, MC_CMD_MAE_GET_OR_CAPS,
> + caps->outer_rule_fields);
> }
>
> /* Bit twiddling:
> @@ -432,11 +446,73 @@ int efx_mae_match_check_caps(struct efx_nic *efx,
> CHECK_BIT(IP_FIRST_FRAG, ip_firstfrag) ||
> CHECK(RECIRC_ID, recirc_id))
> return rc;
> + /* Matches on outer fields are done in a separate hardware table,
> + * the Outer Rule table. Thus the Action Rule merely does an
> + * exact match on Outer Rule ID if any outer field matches are
> + * present. The exception is the VNI/VSID (enc_keyid), which is
> + * available to the Action Rule match iff the Outer Rule matched
if I think :)
> + * (and thus identified the encap protocol to use to extract it).
> + */
> + if (efx_tc_match_is_encap(mask)) {
> + rc = efx_mae_match_check_cap_typ(
> + supported_fields[MAE_FIELD_OUTER_RULE_ID],
> + MASK_ONES);
> + if (rc) {
> + NL_SET_ERR_MSG_MOD(extack, "No support for encap rule ID matches");
> + return rc;
> + }
> + if (CHECK(ENC_VNET_ID, enc_keyid))
> + return rc;
> + } else if (mask->enc_keyid) {
> + NL_SET_ERR_MSG_MOD(extack, "Match on enc_keyid requires other encap fields");
> + return -EINVAL;
> + }
> return 0;
> }
> #undef CHECK_BIT
> #undef CHECK
>
> +#define CHECK(_mcdi) ({ \
> + rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_ ## _mcdi],\
> + MASK_ONES); \
> + if (rc) \
> + NL_SET_ERR_MSG_FMT_MOD(extack, \
> + "No support for field %s", #_mcdi); \
> + rc; \
> +})
Is there any reasone why macro is used instead of function? It is a
little hard to read becasue it is modyfing rc value.
> +/* Checks that the fields needed for encap-rule matches are supported by the
> + * MAE. All the fields are exact-match.
> + */
> +int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
> + struct netlink_ext_ack *extack)
> +{
> + u8 *supported_fields = efx->tc->caps->outer_rule_fields;
> + int rc;
> +
> + if (CHECK(ENC_ETHER_TYPE))
> + return rc;
> + switch (ipv) {
> + case 4:
> + if (CHECK(ENC_SRC_IP4) ||
> + CHECK(ENC_DST_IP4))
> + return rc;
> + break;
> + case 6:
> + if (CHECK(ENC_SRC_IP6) ||
> + CHECK(ENC_DST_IP6))
> + return rc;
> + break;
> + default: /* shouldn't happen */
> + EFX_WARN_ON_PARANOID(1);
> + break;
> + }
> + if (CHECK(ENC_L4_DPORT) ||
> + CHECK(ENC_IP_PROTO))
> + return rc;
> + return 0;
> +}
> +#undef CHECK
> +
> int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt)
> {
> MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_COUNTER_ALLOC_OUT_LEN(1));
> @@ -941,6 +1017,30 @@ static int efx_mae_populate_match_criteria(MCDI_DECLARE_STRUCT_PTR(match_crit),
> match->value.tcp_flags);
> MCDI_STRUCT_SET_WORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_TCP_FLAGS_BE_MASK,
> match->mask.tcp_flags);
> + /* enc-keys are handled indirectly, through encap_match ID */
> + if (match->encap) {
> + MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID,
> + match->encap->fw_id);
> + MCDI_STRUCT_SET_DWORD(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_OUTER_RULE_ID_MASK,
> + (u32)-1);
U32_MAX can't be used here?
> + /* enc_keyid (VNI/VSID) is not part of the encap_match */
> + MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_ENC_VNET_ID_BE,
> + match->value.enc_keyid);
> + MCDI_STRUCT_SET_DWORD_BE(match_crit, MAE_FIELD_MASK_VALUE_PAIRS_V2_ENC_VNET_ID_BE_MASK,
> + match->mask.enc_keyid);
> + } else {
> + /* No enc-keys should appear in a rule without an encap_match */
> + if (WARN_ON_ONCE(match->mask.enc_src_ip) ||
> + WARN_ON_ONCE(match->mask.enc_dst_ip) ||
> + WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_src_ip6)) ||
> + WARN_ON_ONCE(!ipv6_addr_any(&match->mask.enc_dst_ip6)) ||
> + WARN_ON_ONCE(match->mask.enc_ip_tos) ||
> + WARN_ON_ONCE(match->mask.enc_ip_ttl) ||
> + WARN_ON_ONCE(match->mask.enc_sport) ||
> + WARN_ON_ONCE(match->mask.enc_dport) ||
> + WARN_ON_ONCE(match->mask.enc_keyid))
> + return -EOPNOTSUPP;
Can be written as else if {}
Also, You define a similar function: efx_tc_match_is_encap(), I think it
can be used here.
> + }
> return 0;
> }
>
> diff --git a/drivers/net/ethernet/sfc/mae.h b/drivers/net/ethernet/sfc/mae.h
> index bec293a06733..a45d1791517f 100644
> --- a/drivers/net/ethernet/sfc/mae.h
> +++ b/drivers/net/ethernet/sfc/mae.h
> @@ -72,6 +72,7 @@ struct mae_caps {
> u32 match_field_count;
> u32 action_prios;
> u8 action_rule_fields[MAE_NUM_FIELDS];
> + u8 outer_rule_fields[MAE_NUM_FIELDS];
> };
>
> int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps);
> @@ -79,6 +80,8 @@ int efx_mae_get_caps(struct efx_nic *efx, struct mae_caps *caps);
> int efx_mae_match_check_caps(struct efx_nic *efx,
> const struct efx_tc_match_fields *mask,
> struct netlink_ext_ack *extack);
> +int efx_mae_check_encap_match_caps(struct efx_nic *efx, unsigned char ipv,
> + struct netlink_ext_ack *extack);
>
> int efx_mae_allocate_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
> int efx_mae_free_counter(struct efx_nic *efx, struct efx_tc_counter *cnt);
> diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
> index 542853f60c2a..c1485679507c 100644
> --- a/drivers/net/ethernet/sfc/tc.h
> +++ b/drivers/net/ethernet/sfc/tc.h
> @@ -48,11 +48,35 @@ struct efx_tc_match_fields {
> /* L4 */
> __be16 l4_sport, l4_dport; /* Ports (UDP, TCP) */
> __be16 tcp_flags;
> + /* Encap. The following are *outer* fields. Note that there are no
> + * outer eth (L2) fields; this is because TC doesn't have them.
> + */
> + __be32 enc_src_ip, enc_dst_ip;
> + struct in6_addr enc_src_ip6, enc_dst_ip6;
> + u8 enc_ip_tos, enc_ip_ttl;
> + __be16 enc_sport, enc_dport;
> + __be32 enc_keyid; /* e.g. VNI, VSID */
> +};
> +
> +static inline bool efx_tc_match_is_encap(const struct efx_tc_match_fields *mask)
> +{
> + return mask->enc_src_ip || mask->enc_dst_ip ||
> + !ipv6_addr_any(&mask->enc_src_ip6) ||
> + !ipv6_addr_any(&mask->enc_dst_ip6) || mask->enc_ip_tos ||
> + mask->enc_ip_ttl || mask->enc_sport || mask->enc_dport;
> +}
> +
> +struct efx_tc_encap_match {
> + __be32 src_ip, dst_ip;
> + struct in6_addr src_ip6, dst_ip6;
> + __be16 udp_dport;
What about source port? It isn't supported?
> + u32 fw_id; /* index of this entry in firmware encap match table */
> };
>
> struct efx_tc_match {
> struct efx_tc_match_fields value;
> struct efx_tc_match_fields mask;
> + struct efx_tc_encap_match *encap;
> };
>
> struct efx_tc_action_set_list {
Powered by blists - more mailing lists