| 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
| ||
|
Message-ID: <20200419073307.uhm3w2jhsczpchvi@ws.localdomain>
Date: Sun, 19 Apr 2020 09:33:07 +0200
From: "Allan W. Nielsen" <allan.nielsen@...rochip.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: <davem@...emloft.net>, <horatiu.vultur@...rochip.com>,
<alexandre.belloni@...tlin.com>, <antoine.tenart@...tlin.com>,
<andrew@...n.ch>, <f.fainelli@...il.com>,
<vivien.didelot@...il.com>, <joergen.andreasen@...rochip.com>,
<claudiu.manoil@....com>, <netdev@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>, <alexandru.marginean@....com>,
<xiaoliang.yang_1@....com>, <yangbo.lu@....com>, <po.liu@....com>,
<jiri@...lanox.com>, <idosch@...sch.org>, <kuba@...nel.org>
Subject: Re: [PATCH net-next] net: mscc: ocelot: deal with problematic
MAC_ETYPE VCAP IS2 rules
Hi,
Sorry I did not manage to provide feedback before it was merged (I will
need to consult some of my colleagues Monday before I can provide the
foll feedback).
There are many good things in this patch, but it is not only good.
The problem is that these TCAMs/VCAPs are insanely complicated and it is
really hard to make them fit nicely into the existing tc frame-work
(being hard does not mean that we should not try).
In this patch, you try to automatic figure out who the user want the
TCAM to be configured. It works for 1 use-case but it breaks others.
Before this patch you could do a:
tc filter add dev swp0 ingress protocol ipv4 \
flower skip_sw src_ip 10.0.0.1 action drop
tc filter add dev swp0 ingress \
flower skip_sw src_mac 96:18:82:00:04:01 action drop
But the second rule would not apply to the ICMP over IPv4 over Ethernet
packet, it would however apply to non-IP packets.
With this patch it not possible. Your use-case is more common, but the
other one is not unrealistic.
My concern with this, is that I do not think it is possible to automatic
detect how these TCAMs needs to be configured by only looking at the
rules installed by the user. Trying to do this automatic, also makes the
TCAM logic even harder to understand for the user.
I would prefer that we by default uses some conservative default
settings which are easy to understand, and then expose some expert
settings in the sysfs, which can be used to achieve different
behavioral.
Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
understand default.
But I do seem to recall that there is a way to allow matching on both
SMAC and SIP (your original motivation). This may be a better default
(despite that it consumes more TCAM resources). I will follow up and
check if this is possible.
Vladimir (and anyone else whom interested): would you be interested in
spending some time discussion the more high-level architectures and
use-cases on how to best integrate this TCAM architecture into the Linux
kernel. Not sure on the outlook for the various conferences, but we
could arrange some online session to discuss this.
/Allan
On 17.04.2020 22:03, Vladimir Oltean wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>From: Vladimir Oltean <vladimir.oltean@....com>
>
>By default, the VCAP IS2 will produce a single match for each frame, on
>the most specific classification.
>
>Example: a ping packet (ICMP over IPv4 over Ethernet) sent from an IP
>address of 10.0.0.1 and a MAC address of 96:18:82:00:04:01 will match
>this rule:
>
>tc filter add dev swp0 ingress protocol ipv4 \
> flower skip_sw src_ip 10.0.0.1 action drop
>
>but not this one:
>
>tc filter add dev swp0 ingress \
> flower skip_sw src_mac 96:18:82:00:04:01 action drop
>
>Currently the driver does not really warn the user in any way about
>this, and the behavior is rather strange anyway.
>
>The current patch is a workaround to force matches on MAC_ETYPE keys
>(DMAC and SMAC) for all packets irrespective of higher layer protocol.
>The setting is made at the port level.
>
>Of course this breaks all other non-src_mac and non-dst_mac matches, so
>rule exclusivity checks have been added to the driver, in order to never
>have rules of both types on any ingress port.
>
>The bits that discard higher-level protocol information are set only
>once a MAC_ETYPE rule is added to a filter block, and only for the ports
>that are bound to that filter block. Then all further non-MAC_ETYPE
>rules added to that filter block should be denied by the ports bound to
>it.
>
>Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>---
> drivers/net/ethernet/mscc/ocelot_ace.c | 103 +++++++++++++++++++++-
> drivers/net/ethernet/mscc/ocelot_ace.h | 5 +-
> drivers/net/ethernet/mscc/ocelot_flower.c | 2 +-
> 3 files changed, 106 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
>index 3bd286044480..8a2f7d13ef6d 100644
>--- a/drivers/net/ethernet/mscc/ocelot_ace.c
>+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
>@@ -706,13 +706,114 @@ ocelot_ace_rule_get_rule_index(struct ocelot_acl_block *block, int index)
> return NULL;
> }
>
>+/* If @on=false, then SNAP, ARP, IP and OAM frames will not match on keys based
>+ * on destination and source MAC addresses, but only on higher-level protocol
>+ * information. The only frame types to match on keys containing MAC addresses
>+ * in this case are non-SNAP, non-ARP, non-IP and non-OAM frames.
>+ *
>+ * If @on=true, then the above frame types (SNAP, ARP, IP and OAM) will match
>+ * on MAC_ETYPE keys such as destination and source MAC on this ingress port.
>+ * However the setting has the side effect of making these frames not matching
>+ * on any _other_ keys than MAC_ETYPE ones.
>+ */
>+static void ocelot_match_all_as_mac_etype(struct ocelot *ocelot, int port,
>+ bool on)
>+{
>+ u32 val = 0;
>+
>+ if (on)
>+ val = ANA_PORT_VCAP_S2_CFG_S2_SNAP_DIS(3) |
>+ ANA_PORT_VCAP_S2_CFG_S2_ARP_DIS(3) |
>+ ANA_PORT_VCAP_S2_CFG_S2_IP_TCPUDP_DIS(3) |
>+ ANA_PORT_VCAP_S2_CFG_S2_IP_OTHER_DIS(3) |
>+ ANA_PORT_VCAP_S2_CFG_S2_OAM_DIS(3);
>+
>+ ocelot_rmw_gix(ocelot, val,
>+ ANA_PORT_VCAP_S2_CFG_S2_SNAP_DIS_M |
>+ ANA_PORT_VCAP_S2_CFG_S2_ARP_DIS_M |
>+ ANA_PORT_VCAP_S2_CFG_S2_IP_TCPUDP_DIS_M |
>+ ANA_PORT_VCAP_S2_CFG_S2_IP_OTHER_DIS_M |
>+ ANA_PORT_VCAP_S2_CFG_S2_OAM_DIS_M,
>+ ANA_PORT_VCAP_S2_CFG, port);
>+}
>+
>+static bool ocelot_ace_is_problematic_mac_etype(struct ocelot_ace_rule *ace)
>+{
>+ if (ace->type != OCELOT_ACE_TYPE_ETYPE)
>+ return false;
>+ if (ether_addr_to_u64(ace->frame.etype.dmac.value) &
>+ ether_addr_to_u64(ace->frame.etype.dmac.mask))
>+ return true;
>+ if (ether_addr_to_u64(ace->frame.etype.smac.value) &
>+ ether_addr_to_u64(ace->frame.etype.smac.mask))
>+ return true;
>+ return false;
>+}
>+
>+static bool ocelot_ace_is_problematic_non_mac_etype(struct ocelot_ace_rule *ace)
>+{
>+ if (ace->type == OCELOT_ACE_TYPE_SNAP)
>+ return true;
>+ if (ace->type == OCELOT_ACE_TYPE_ARP)
>+ return true;
>+ if (ace->type == OCELOT_ACE_TYPE_IPV4)
>+ return true;
>+ if (ace->type == OCELOT_ACE_TYPE_IPV6)
>+ return true;
>+ return false;
>+}
>+
>+static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot,
>+ struct ocelot_ace_rule *ace)
>+{
>+ struct ocelot_acl_block *block = &ocelot->acl_block;
>+ struct ocelot_ace_rule *tmp;
>+ unsigned long port;
>+ int i;
>+
>+ if (ocelot_ace_is_problematic_mac_etype(ace)) {
>+ /* Search for any non-MAC_ETYPE rules on the port */
>+ for (i = 0; i < block->count; i++) {
>+ tmp = ocelot_ace_rule_get_rule_index(block, i);
>+ if (tmp->ingress_port_mask & ace->ingress_port_mask &&
>+ ocelot_ace_is_problematic_non_mac_etype(tmp))
>+ return false;
>+ }
>+
>+ for_each_set_bit(port, &ace->ingress_port_mask,
>+ ocelot->num_phys_ports)
>+ ocelot_match_all_as_mac_etype(ocelot, port, true);
>+ } else if (ocelot_ace_is_problematic_non_mac_etype(ace)) {
>+ /* Search for any MAC_ETYPE rules on the port */
>+ for (i = 0; i < block->count; i++) {
>+ tmp = ocelot_ace_rule_get_rule_index(block, i);
>+ if (tmp->ingress_port_mask & ace->ingress_port_mask &&
>+ ocelot_ace_is_problematic_mac_etype(tmp))
>+ return false;
>+ }
>+
>+ for_each_set_bit(port, &ace->ingress_port_mask,
>+ ocelot->num_phys_ports)
>+ ocelot_match_all_as_mac_etype(ocelot, port, false);
>+ }
>+
>+ return true;
>+}
>+
> int ocelot_ace_rule_offload_add(struct ocelot *ocelot,
>- struct ocelot_ace_rule *rule)
>+ struct ocelot_ace_rule *rule,
>+ struct netlink_ext_ack *extack)
> {
> struct ocelot_acl_block *block = &ocelot->acl_block;
> struct ocelot_ace_rule *ace;
> int i, index;
>
>+ if (!ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) {
>+ NL_SET_ERR_MSG_MOD(extack,
>+ "Cannot mix MAC_ETYPE with non-MAC_ETYPE rules");
>+ return -EBUSY;
>+ }
>+
> /* Add rule to the linked list */
> ocelot_ace_rule_add(ocelot, block, rule);
>
>diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h
>index 29d22c566786..099e177f2617 100644
>--- a/drivers/net/ethernet/mscc/ocelot_ace.h
>+++ b/drivers/net/ethernet/mscc/ocelot_ace.h
>@@ -194,7 +194,7 @@ struct ocelot_ace_rule {
>
> enum ocelot_ace_action action;
> struct ocelot_ace_stats stats;
>- u16 ingress_port_mask;
>+ unsigned long ingress_port_mask;
>
> enum ocelot_vcap_bit dmac_mc;
> enum ocelot_vcap_bit dmac_bc;
>@@ -215,7 +215,8 @@ struct ocelot_ace_rule {
> };
>
> int ocelot_ace_rule_offload_add(struct ocelot *ocelot,
>- struct ocelot_ace_rule *rule);
>+ struct ocelot_ace_rule *rule,
>+ struct netlink_ext_ack *extack);
> int ocelot_ace_rule_offload_del(struct ocelot *ocelot,
> struct ocelot_ace_rule *rule);
> int ocelot_ace_rule_stats_update(struct ocelot *ocelot,
>diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
>index 341923311fec..954cb67eeaa2 100644
>--- a/drivers/net/ethernet/mscc/ocelot_flower.c
>+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
>@@ -205,7 +205,7 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port,
> return ret;
> }
>
>- return ocelot_ace_rule_offload_add(ocelot, ace);
>+ return ocelot_ace_rule_offload_add(ocelot, ace, f->common.extack);
> }
> EXPORT_SYMBOL_GPL(ocelot_cls_flower_replace);
>
>--
>2.17.1
>
/Allan
Powered by blists - more mailing lists