netdev - Re: [PATCH net-next v2 1/1] net: mscc: ocelot: Implement port policers via tc command

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190523115630.7710cc49@cakuba.netronome.com>
Date:   Thu, 23 May 2019 11:56:46 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Joergen Andreasen <joergen.andreasen@...rochip.com>
Cc:     <netdev@...r.kernel.org>,
        "Microchip Linux Driver Support" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "David S. Miller" <davem@...emloft.net>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/1] net: mscc: ocelot: Implement port
 policers via tc command

On Thu, 23 May 2019 12:49:39 +0200, Joergen Andreasen wrote:
> Hardware offload of matchall classifier and police action are now
> supported via the tc command.
> Supported police parameters are: rate and burst.
> 
> Example:
> 
> Add:
> tc qdisc add dev eth3 handle ffff: ingress
> tc filter add dev eth3 parent ffff: prio 1 handle 2	\
> 	matchall skip_sw				\
> 	action police rate 100Mbit burst 10000
> 
> Show:
> tc -s -d qdisc show dev eth3
> tc -s -d filter show dev eth3 ingress
> 
> Delete:
> tc filter del dev eth3 parent ffff: prio 1
> tc qdisc del dev eth3 handle ffff: ingress
> 
> Signed-off-by: Joergen Andreasen <joergen.andreasen@...rochip.com>

> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index d715ef4fc92f..3ec7864d9dc8 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -943,6 +943,7 @@ static const struct net_device_ops ocelot_port_netdev_ops = {
>  	.ndo_vlan_rx_kill_vid		= ocelot_vlan_rx_kill_vid,
>  	.ndo_set_features		= ocelot_set_features,
>  	.ndo_get_port_parent_id		= ocelot_get_port_parent_id,
> +	.ndo_setup_tc			= ocelot_setup_tc,
>  };
>  
>  static void ocelot_get_strings(struct net_device *netdev, u32 sset, u8 *data)
> @@ -1663,8 +1664,9 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
>  	dev->netdev_ops = &ocelot_port_netdev_ops;
>  	dev->ethtool_ops = &ocelot_ethtool_ops;
>  
> -	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS;
> -	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS |
> +		NETIF_F_HW_TC;
> +	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_TC;
>  
>  	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
>  	dev->dev_addr[ETH_ALEN - 1] += port;

You need to add a check in set_features to make sure nobody clears the
NETIF_F_TC flag while something is offloaded, otherwise you will miss
the REMOVE callback (it will bounce from the
tc_cls_can_offload_and_chain0() check).

> diff --git a/drivers/net/ethernet/mscc/ocelot_tc.c b/drivers/net/ethernet/mscc/ocelot_tc.c
> new file mode 100644
> index 000000000000..2412e0dbc267
> --- /dev/null
> +++ b/drivers/net/ethernet/mscc/ocelot_tc.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Microsemi Ocelot Switch TC driver
> + *
> + * Copyright (c) 2019 Microsemi Corporation
> + */
> +
> +#include "ocelot_tc.h"
> +#include "ocelot_police.h"
> +#include <net/pkt_cls.h>
> +
> +static int ocelot_setup_tc_cls_matchall(struct ocelot_port *port,
> +					struct tc_cls_matchall_offload *f,
> +					bool ingress)
> +{
> +	struct netlink_ext_ack *extack = f->common.extack;
> +	struct ocelot_policer pol = { 0 };
> +	struct flow_action_entry *action;
> +	int err;
> +
> +	netdev_dbg(port->dev, "%s: port %u command %d cookie %lu\n",
> +		   __func__, port->chip_port, f->command, f->cookie);
> +
> +	if (!ingress) {
> +		NL_SET_ERR_MSG_MOD(extack, "Only ingress is supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	switch (f->command) {
> +	case TC_CLSMATCHALL_REPLACE:
> +		if (!flow_offload_has_one_action(&f->rule->action)) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Only one action is supported");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		action = &f->rule->action.entries[0];
> +
> +		if (action->id != FLOW_ACTION_POLICE) {
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
> +			return -EOPNOTSUPP;
> +		}

Please also reject the offload if block is shared, as HW policer state
cannot be shared between ports, the way it is in SW.  You have to save
whether the block is shared or not at bind time, see:

d6787147e15d ("net/sched: remove block pointer from common offload structure")

> +		if (port->tc.police_id && port->tc.police_id != f->cookie) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Only one policer per port is supported\n");
> +			return -EEXIST;
> +		}
> +
> +		pol.rate = (u32)div_u64(action->police.rate_bytes_ps, 1000) * 8;
> +		pol.burst = (u32)div_u64(action->police.rate_bytes_ps *
> +					 PSCHED_NS2TICKS(action->police.burst),
> +					 PSCHED_TICKS_PER_SEC);
> +
> +		err = ocelot_port_policer_add(port, &pol);
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(extack, "Could not add policer\n");
> +			return err;
> +		}
> +
> +		port->tc.police_id = f->cookie;
> +		return 0;
> +	case TC_CLSMATCHALL_DESTROY:
> +		if (port->tc.police_id != f->cookie)
> +			return -ENOENT;
> +
> +		err = ocelot_port_policer_del(port);
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Could not delete policer\n");
> +			return err;
> +		}
> +		port->tc.police_id = 0;
> +		return 0;
> +	case TC_CLSMATCHALL_STATS: /* fall through */
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}