| 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, 22 Feb 2022 01:58:23 +0000
From: Jianbo Liu <jianbol@...dia.com>
To: "olteanv@...il.com" <olteanv@...il.com>
CC: "andrew@...n.ch" <andrew@...n.ch>,
"claudiu.manoil@....com" <claudiu.manoil@....com>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>,
Petr Machata <petrm@...dia.com>,
"jhs@...atatu.com" <jhs@...atatu.com>,
"oss-drivers@...igine.com" <oss-drivers@...igine.com>,
"hkelam@...vell.com" <hkelam@...vell.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"leon@...nel.org" <leon@...nel.org>,
"peng.zhang@...igine.com" <peng.zhang@...igine.com>,
"louis.peens@...ronome.com" <louis.peens@...ronome.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
"rajur@...lsio.com" <rajur@...lsio.com>,
Ido Schimmel <idosch@...dia.com>,
"simon.horman@...igine.com" <simon.horman@...igine.com>,
"sbhatta@...vell.com" <sbhatta@...vell.com>,
"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
Roi Dayan <roid@...dia.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"baowen.zheng@...igine.com" <baowen.zheng@...igine.com>,
"jiri@...nulli.us" <jiri@...nulli.us>,
Saeed Mahameed <saeedm@...dia.com>,
"sgoutham@...vell.com" <sgoutham@...vell.com>,
"gakula@...vell.com" <gakula@...vell.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"vladimir.oltean@....com" <vladimir.oltean@....com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/2] flow_offload: reject offload for all
drivers with invalid police parameters
On Thu, 2022-02-17 at 14:49 +0200, Vladimir Oltean wrote:
> On Thu, Feb 17, 2022 at 08:28:03AM +0000, Jianbo Liu wrote:
> > As more police parameters are passed to flow_offload, driver can
> > check
> > them to make sure hardware handles packets in the way indicated by
> > tc.
> > The conform-exceed control should be drop/pipe or drop/ok. Besides,
> > for drop/ok, the police should be the last action. As hardware
> > can't
> > configure peakrate/avrate/overhead, offload should not be supported
> > if
> > any of them is configured.
> >
> > Signed-off-by: Jianbo Liu <jianbol@...dia.com>
> > Reviewed-by: Roi Dayan <roid@...dia.com>
> > Reviewed-by: Ido Schimmel <idosch@...dia.com>
> > ---
>
> Tested-by: Vladimir Oltean <vladimir.oltean@....com>
>
> But could we cut down on line length a little? Example for sja1105
> (messages were also shortened):
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c
> b/drivers/net/dsa/sja1105/sja1105_flower.c
> index 8a14df8cf91e..54a16369a39e 100644
> --- a/drivers/net/dsa/sja1105/sja1105_flower.c
> +++ b/drivers/net/dsa/sja1105/sja1105_flower.c
> @@ -300,6 +300,46 @@ static int sja1105_flower_parse_key(struct
> sja1105_private *priv,
> return -EOPNOTSUPP;
> }
>
> +static int sja1105_policer_validate(const struct flow_action
> *action,
> + const struct flow_action_entry
> *act,
> + struct netlink_ext_ack *extack)
> +{
> + if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Offload not supported when exceed
> action is not drop");
> + return -EOPNOTSUPP;
> + }
> +
> + if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
> + act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Offload not supported when
> conform action is not pipe or ok");
> + return -EOPNOTSUPP;
> + }
> +
> + if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
> + !flow_action_is_last_entry(action, act)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Offload not supported when
> conform action is ok, but action is not last");
> + return -EOPNOTSUPP;
> + }
> +
> + if (act->police.peakrate_bytes_ps ||
> + act->police.avrate || act->police.overhead) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Offload not supported when
> peakrate/avrate/overhead is configured");
> + return -EOPNOTSUPP;
> + }
> +
> + if (act->police.rate_pkt_ps) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "QoS offload not support packets
> per second");
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> int sja1105_cls_flower_add(struct dsa_switch *ds, int port,
> struct flow_cls_offload *cls, bool
> ingress)
> {
> @@ -321,39 +361,10 @@ int sja1105_cls_flower_add(struct dsa_switch
> *ds, int port,
> flow_action_for_each(i, act, &rule->action) {
> switch (act->id) {
> case FLOW_ACTION_POLICE:
> - if (act->police.exceed.act_id !=
> FLOW_ACTION_DROP) {
> - NL_SET_ERR_MSG_MOD(extack,
> - "Police offload is
> not supported when the exceed action is not drop");
> - return -EOPNOTSUPP;
> - }
> -
> - if (act->police.notexceed.act_id !=
> FLOW_ACTION_PIPE &&
> - act->police.notexceed.act_id !=
> FLOW_ACTION_ACCEPT) {
> - NL_SET_ERR_MSG_MOD(extack,
> - "Police offload is
> not supported when the conform action is not pipe or ok");
> - return -EOPNOTSUPP;
> - }
> -
> - if (act->police.notexceed.act_id ==
> FLOW_ACTION_ACCEPT &&
> - !flow_action_is_last_entry(&rule->action,
> act)) {
> - NL_SET_ERR_MSG_MOD(extack,
> - "Police offload is
> not supported when the conform action is ok, but police action is not
> last");
> - return -EOPNOTSUPP;
> - }
> -
> - if (act->police.peakrate_bytes_ps ||
> - act->police.avrate || act-
> >police.overhead) {
> - NL_SET_ERR_MSG_MOD(extack,
> - "Police offload is
> not supported when peakrate/avrate/overhead is configured");
> - return -EOPNOTSUPP;
> - }
> -
> - if (act->police.rate_pkt_ps) {
> - NL_SET_ERR_MSG_MOD(extack,
> - "QoS offload not
> support packets per second");
> - rc = -EOPNOTSUPP;
> + rc = sja1105_policer_validate(&rule->action,
> act,
> + extack);
> + if (rc)
> goto out;
> - }
>
> rc = sja1105_flower_policer(priv, port,
> extack, cookie,
> &key,
>
> Also, if you create a "validate" function for every driver, you'll
> remove code duplication for those drivers that support both matchall
> and
> flower policers.
Hi Vladimir,
I'd love to hear your suggestion regarding where this validate function
to be placed for drivers/net/ethernet/mscc, as it will be used by both
ocelot_net.c and ocelot_flower.c.
Thanks!
Jianbo
Powered by blists - more mailing lists