[PATCH net-next v6] net: sched: Introduce act_ctinfo action

[PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Kevin 'ldir' Darbyshire-Bryant]

ctinfo is a new tc filter action module.  It is designed to restore
information contained in firewall conntrack marks to other packet fields
and is typically used on packet ingress paths.  At present it has two
independent sub-functions or operating modes, DSCP restoration mode &
skb mark restoration mode.

The DSCP restore mode:

This mode copies DSCP values that have been placed in the firewall
conntrack mark back into the IPv4/v6 diffserv fields of relevant
packets.

The DSCP restoration is intended for use and has been found useful for
restoring ingress classifications based on egress classifications across
links that bleach or otherwise change DSCP, typically home ISP Internet
links.  Restoring DSCP on ingress on the WAN link allows qdiscs such as
but by no means limited to CAKE to shape inbound packets according to
policies that are easier to set & mark on egress.

Ingress classification is traditionally a challenging task since
iptables rules haven't yet run and tc filter/eBPF programs are pre-NAT
lookups, hence are unable to see internal IPv4 addresses as used on the
typical home masquerading gateway.  Thus marking the connection in some
manner on egress for later restoration of classification on ingress is
easier to implement.

Parameters related to DSCP restore mode:

dscpmask - a 32 bit mask of 6 contiguous bits and indicate bits of the
conntrack mark field contain the DSCP value to be restored.

statemask - a 32 bit mask of (usually) 1 bit length, outside the area
specified by dscpmask.  This represents a conditional operation flag
whereby the DSCP is only restored if the flag is set.  This is useful to
implement a 'one shot' iptables based classification where the
'complicated' iptables rules are only run once to classify the
connection on initial (egress) packet and subsequent packets are all
marked/restored with the same DSCP.  A mask of zero disables the
conditional behaviour ie. the conntrack mark DSCP bits are always
restored to the ip diffserv field (assuming the conntrack entry is found
& the skb is an ipv4/ipv6 type)

e.g. dscpmask 0xfc000000 statemask 0x01000000

|----0xFC----conntrack mark----000000---|
| Bits 31-26 | bit 25 | bit24 |~~~ Bit 0|
| DSCP       | unused | flag  |unused   |
|-----------------------0x01---000000---|
      |                   |
      |                   |
      ---|             Conditional flag
         v             only restore if set
|-ip diffserv-|
| 6 bits      |
|-------------|

The skb mark restore mode (cpmark):

This mode copies the firewall conntrack mark to the skb's mark field.
It is completely the functional equivalent of the existing act_connmark
action with the additional feature of being able to apply a mask to the
restored value.

Parameters related to skb mark restore mode:

mask - a 32 bit mask applied to the firewall conntrack mark to mask out
bits unwanted for restoration.  This can be useful where the conntrack
mark is being used for different purposes by different applications.  If
not specified and by default the whole mark field is copied (i.e.
default mask of 0xffffffff)

e.g. mask 0x00ffffff to mask out the top 8 bits being used by the
aforementioned DSCP restore mode.

|----0x00----conntrack mark----ffffff---|
| Bits 31-24 |                          |
| DSCP & flag|      some value here     |
|---------------------------------------|
			|
			|
			v
|------------skb mark-------------------|
|            |                          |
|  zeroed    |                          |
|---------------------------------------|

Overall parameters:

zone - conntrack zone

control - action related control (reclassify | pipe | drop | continue |
ok | goto chain <CHAIN_INDEX>)

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>

---
v2 - add equivalent connmark functionality with an enhancement
    to accept a mask
    pass statistics for each sub-function as individual netlink
    attributes and stop (ab)using overlimits, drops
    update the testing config correctly
v3 - fix a licensing silly & tidy up GPL boilerplate
v4 - drop stray copy paste inline
    reverse christmas tree local vars
v5 - rebase on net-next/master not net/master by mistake - doh! now
     applies!
     rename connmark to cpmark.
     always use structures across netlink to pass parameters.
     rework commit message to clarify modes & applicable parameters
     without getting bogged down in userspace syntax.
     restrict dscpmask parameter to 6 contiguous bits only instead
     of >=6 contiguous bits.
     re-order netlink TLV values into functional groupings.
v6 - don't use structs across netlink to pass parameter & stats, use
     individual TLVs instead.
     always pass the zone parameter across netlink ie. even the 0
     default zone.
     pass all statistics even if not using particular sub function/mode.
     drop the MODE selector TLVs as the operating mode/s are implied by
     the first parameter of each sub-function being present.

 include/net/tc_act/tc_ctinfo.h            |  28 +++
 include/uapi/linux/pkt_cls.h              |   1 +
 include/uapi/linux/tc_act/tc_ctinfo.h     |  34 +++
 net/sched/Kconfig                         |  17 ++
 net/sched/Makefile                        |   1 +
 net/sched/act_ctinfo.c                    | 396 ++++++++++++++++++++++++++++++
 tools/testing/selftests/tc-testing/config |   1 +
 7 files changed, 478 insertions(+)
 create mode 100644 include/net/tc_act/tc_ctinfo.h
 create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
 create mode 100644 net/sched/act_ctinfo.c

diff --git a/include/net/tc_act/tc_ctinfo.h b/include/net/tc_act/tc_ctinfo.h
new file mode 100644
index 000000000000..d6a688571672
--- /dev/null
+++ b/include/net/tc_act/tc_ctinfo.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __NET_TC_CTINFO_H
+#define __NET_TC_CTINFO_H
+
+#include <net/act_api.h>
+
+struct tcf_ctinfo_params {
+	struct rcu_head rcu;
+	struct net *net;
+	u32 dscpmask;
+	u32 dscpstatemask;
+	u32 cpmarkmask;
+	u16 zone;
+	u8 mode;
+	u8 dscpmaskshift;
+};
+
+struct tcf_ctinfo {
+	struct tc_action common;
+	struct tcf_ctinfo_params __rcu *params;
+	u64 stats_dscp_set;
+	u64 stats_dscp_error;
+	u64 stats_cpmark_set;
+};
+
+#define to_ctinfo(a) ((struct tcf_ctinfo *)a)
+
+#endif /* __NET_TC_CTINFO_H */
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 51a0496f78ea..a93680fc4bfa 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -105,6 +105,7 @@ enum tca_id {
 	TCA_ID_IFE = TCA_ACT_IFE,
 	TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
 	/* other actions go here */
+	TCA_ID_CTINFO,
 	__TCA_ID_MAX = 255
 };
 
diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
new file mode 100644
index 000000000000..da803e05a89b
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_ctinfo.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __UAPI_TC_CTINFO_H
+#define __UAPI_TC_CTINFO_H
+
+#include <linux/types.h>
+#include <linux/pkt_cls.h>
+
+struct tc_ctinfo {
+	tc_gen;
+};
+
+enum {
+	TCA_CTINFO_UNSPEC,
+	TCA_CTINFO_PAD,
+	TCA_CTINFO_TM,
+	TCA_CTINFO_ACT,
+	TCA_CTINFO_ZONE,
+	TCA_CTINFO_PARMS_DSCP_MASK,
+	TCA_CTINFO_PARMS_DSCP_STATEMASK,
+	TCA_CTINFO_PARMS_CPMARK_MASK,
+	TCA_CTINFO_STATS_DSCP_SET,
+	TCA_CTINFO_STATS_DSCP_ERROR,
+	TCA_CTINFO_STATS_CPMARK_SET,
+	__TCA_CTINFO_MAX
+};
+
+#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
+
+enum {
+	CTINFO_MODE_DSCP	= BIT(0),
+	CTINFO_MODE_CPMARK	= BIT(1)
+};
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index 2c72d95c3050..d104f7ee26c7 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -877,6 +877,23 @@ config NET_ACT_CONNMARK
 	  To compile this code as a module, choose M here: the
 	  module will be called act_connmark.
 
+config NET_ACT_CTINFO
+        tristate "Netfilter Connection Mark Actions"
+        depends on NET_CLS_ACT && NETFILTER && IP_NF_IPTABLES
+        depends on NF_CONNTRACK && NF_CONNTRACK_MARK
+        help
+	  Say Y here to allow transfer of a connmark stored information.
+	  Current actions transfer connmark stored DSCP into
+	  ipv4/v6 diffserv and/or to transfer connmark to packet
+	  mark.  Both are useful for restoring egress based marks
+	  back onto ingress connections for qdisc priority mapping
+	  purposes.
+
+	  If unsure, say N.
+
+	  To compile this code as a module, choose M here: the
+	  module will be called act_ctinfo.
+
 config NET_ACT_SKBMOD
         tristate "skb data modification action"
         depends on NET_CLS_ACT
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 8a40431d7b5c..d54bfcbd7981 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_NET_ACT_CSUM)	+= act_csum.o
 obj-$(CONFIG_NET_ACT_VLAN)	+= act_vlan.o
 obj-$(CONFIG_NET_ACT_BPF)	+= act_bpf.o
 obj-$(CONFIG_NET_ACT_CONNMARK)	+= act_connmark.o
+obj-$(CONFIG_NET_ACT_CTINFO)	+= act_ctinfo.o
 obj-$(CONFIG_NET_ACT_SKBMOD)	+= act_skbmod.o
 obj-$(CONFIG_NET_ACT_IFE)	+= act_ife.o
 obj-$(CONFIG_NET_IFE_SKBMARK)	+= act_meta_mark.o
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
new file mode 100644
index 000000000000..926109139a81
--- /dev/null
+++ b/net/sched/act_ctinfo.c
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* net/sched/act_ctinfo.c  netfilter ctinfo connmark actions
+ *
+ * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/pkt_cls.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+#include <net/act_api.h>
+#include <net/pkt_cls.h>
+#include <uapi/linux/tc_act/tc_ctinfo.h>
+#include <net/tc_act/tc_ctinfo.h>
+
+#include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_conntrack_ecache.h>
+#include <net/netfilter/nf_conntrack_zones.h>
+
+static struct tc_action_ops act_ctinfo_ops;
+static unsigned int ctinfo_net_id;
+
+static void tcf_ctinfo_dscp_set(struct nf_conn *ct, struct tcf_ctinfo *ca,
+				struct tcf_ctinfo_params *cp,
+				struct sk_buff *skb, int wlen, int proto)
+{
+	u8 dscp, newdscp;
+
+	newdscp = (((ct->mark & cp->dscpmask) >> cp->dscpmaskshift) << 2) &
+		     ~INET_ECN_MASK;
+
+	switch (proto) {
+	case NFPROTO_IPV4:
+		dscp = ipv4_get_dsfield(ip_hdr(skb)) & ~INET_ECN_MASK;
+		if (dscp != newdscp) {
+			if (likely(!skb_try_make_writable(skb, wlen))) {
+				ipv4_change_dsfield(ip_hdr(skb),
+						    INET_ECN_MASK,
+						    newdscp);
+				ca->stats_dscp_set++;
+			} else {
+				ca->stats_dscp_error++;
+			}
+		}
+		break;
+	case NFPROTO_IPV6:
+		dscp = ipv6_get_dsfield(ipv6_hdr(skb)) & ~INET_ECN_MASK;
+		if (dscp != newdscp) {
+			if (likely(!skb_try_make_writable(skb, wlen))) {
+				ipv6_change_dsfield(ipv6_hdr(skb),
+						    INET_ECN_MASK,
+						    newdscp);
+				ca->stats_dscp_set++;
+			} else {
+				ca->stats_dscp_error++;
+			}
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+static void tcf_ctinfo_cpmark_set(struct nf_conn *ct, struct tcf_ctinfo *ca,
+				  struct tcf_ctinfo_params *cp,
+				  struct sk_buff *skb)
+{
+	ca->stats_cpmark_set++;
+	skb->mark = ct->mark & cp->cpmarkmask;
+}
+
+static int tcf_ctinfo_act(struct sk_buff *skb, const struct tc_action *a,
+			  struct tcf_result *res)
+{
+	const struct nf_conntrack_tuple_hash *thash = NULL;
+	struct tcf_ctinfo *ca = to_ctinfo(a);
+	struct nf_conntrack_tuple tuple;
+	struct nf_conntrack_zone zone;
+	enum ip_conntrack_info ctinfo;
+	struct tcf_ctinfo_params *cp;
+	struct nf_conn *ct;
+	int proto, wlen;
+	int action;
+
+	cp = rcu_dereference_bh(ca->params);
+
+	tcf_lastuse_update(&ca->tcf_tm);
+	bstats_update(&ca->tcf_bstats, skb);
+	action = READ_ONCE(ca->tcf_action);
+
+	wlen = skb_network_offset(skb);
+	if (tc_skb_protocol(skb) == htons(ETH_P_IP)) {
+		wlen += sizeof(struct iphdr);
+		if (!pskb_may_pull(skb, wlen))
+			goto out;
+
+		proto = NFPROTO_IPV4;
+	} else if (tc_skb_protocol(skb) == htons(ETH_P_IPV6)) {
+		wlen += sizeof(struct ipv6hdr);
+		if (!pskb_may_pull(skb, wlen))
+			goto out;
+
+		proto = NFPROTO_IPV6;
+	} else {
+		goto out;
+	}
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct) { /* look harder, usually ingress */
+		if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
+				       proto, cp->net, &tuple))
+			goto out;
+		zone.id = cp->zone;
+		zone.dir = NF_CT_DEFAULT_ZONE_DIR;
+
+		thash = nf_conntrack_find_get(cp->net, &zone, &tuple);
+		if (!thash)
+			goto out;
+
+		ct = nf_ct_tuplehash_to_ctrack(thash);
+	}
+
+	if (cp->mode & CTINFO_MODE_DSCP)
+		if (!cp->dscpstatemask || (ct->mark & cp->dscpstatemask))
+			tcf_ctinfo_dscp_set(ct, ca, cp, skb, wlen, proto);
+
+	if (cp->mode & CTINFO_MODE_CPMARK)
+		tcf_ctinfo_cpmark_set(ct, ca, cp, skb);
+
+	if (thash)
+		nf_ct_put(ct);
+out:
+	return action;
+}
+
+static const struct nla_policy ctinfo_policy[TCA_CTINFO_MAX + 1] = {
+	[TCA_CTINFO_ACT]		  = { .len = sizeof(struct
+							    tc_ctinfo) },
+	[TCA_CTINFO_ZONE]		  = { .type = NLA_U16 },
+	[TCA_CTINFO_PARMS_DSCP_MASK]	  = { .type = NLA_U32 },
+	[TCA_CTINFO_PARMS_DSCP_STATEMASK] = { .type = NLA_U32 },
+	[TCA_CTINFO_PARMS_CPMARK_MASK]	  = { .type = NLA_U32 },
+};
+
+static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
+			   struct nlattr *est, struct tc_action **a,
+			   int ovr, int bind, bool rtnl_held,
+			   struct tcf_proto *tp,
+			   struct netlink_ext_ack *extack)
+{
+	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
+	struct nlattr *tb[TCA_CTINFO_MAX + 1];
+	struct tcf_ctinfo_params *cp_new;
+	struct tcf_chain *goto_ch = NULL;
+	u32 dscpmask = 0, dscpstatemask;
+	struct tc_ctinfo *actparm;
+	struct tcf_ctinfo *ci;
+	u8 dscpmaskshift;
+	int ret = 0, err;
+
+	if (!nla)
+		return -EINVAL;
+
+	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
+	if (err < 0)
+		return err;
+
+	if (!tb[TCA_CTINFO_ACT])
+		return -EINVAL;
+	actparm = nla_data(tb[TCA_CTINFO_ACT]);
+
+	/* do some basic validation here before dynamically allocating things */
+	/* that we would otherwise have to clean up.			      */
+	if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
+		dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
+		/* need contiguous 6 bit mask */
+		dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
+		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
+			return -EINVAL;
+		dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
+			nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
+		/* mask & statemask must not overlap */
+		if (dscpmask & dscpstatemask)
+			return -EINVAL;
+	}
+
+	/* done the validation:now to the actual action allocation */
+	err = tcf_idr_check_alloc(tn, &actparm->index, a, bind);
+	if (!err) {
+		ret = tcf_idr_create(tn, actparm->index, est, a,
+				     &act_ctinfo_ops, bind, false);
+		if (ret) {
+			tcf_idr_cleanup(tn, actparm->index);
+			return ret;
+		}
+	} else if (err > 0) {
+		if (bind) /* don't override defaults */
+			return 0;
+		if (!ovr) {
+			tcf_idr_release(*a, bind);
+			return -EEXIST;
+		}
+	} else {
+		return err;
+	}
+
+	err = tcf_action_check_ctrlact(actparm->action, tp, &goto_ch, extack);
+	if (err < 0)
+		goto release_idr;
+
+	ci = to_ctinfo(*a);
+
+	cp_new = kzalloc(sizeof(*cp_new), GFP_KERNEL);
+	if (unlikely(!cp_new)) {
+		err = -ENOMEM;
+		goto put_chain;
+	}
+
+	cp_new->net = net;
+	cp_new->zone = tb[TCA_CTINFO_ZONE] ?
+			nla_get_u16(tb[TCA_CTINFO_ZONE]) : 0;
+	if (dscpmask) {
+		cp_new->dscpmask = dscpmask;
+		cp_new->dscpmaskshift = dscpmaskshift;
+		cp_new->dscpstatemask = dscpstatemask;
+		cp_new->mode |= CTINFO_MODE_DSCP;
+	} else {
+		cp_new->mode &= ~CTINFO_MODE_DSCP;
+	}
+
+	if (tb[TCA_CTINFO_PARMS_CPMARK_MASK]) {
+		cp_new->cpmarkmask =
+				nla_get_u32(tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
+		cp_new->mode |= CTINFO_MODE_CPMARK;
+	} else {
+		cp_new->mode &= ~CTINFO_MODE_CPMARK;
+	}
+
+	spin_lock_bh(&ci->tcf_lock);
+	goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
+	rcu_swap_protected(ci->params, cp_new,
+			   lockdep_is_held(&ci->tcf_lock));
+	spin_unlock_bh(&ci->tcf_lock);
+
+	if (goto_ch)
+		tcf_chain_put_by_act(goto_ch);
+	if (cp_new)
+		kfree_rcu(cp_new, rcu);
+
+	if (ret == ACT_P_CREATED)
+		tcf_idr_insert(tn, *a);
+
+	return ret;
+
+put_chain:
+	if (goto_ch)
+		tcf_chain_put_by_act(goto_ch);
+release_idr:
+	tcf_idr_release(*a, bind);
+	return err;
+}
+
+static int tcf_ctinfo_dump(struct sk_buff *skb, struct tc_action *a,
+			   int bind, int ref)
+{
+	struct tcf_ctinfo *ci = to_ctinfo(a);
+	struct tc_ctinfo opt = {
+		.index   = ci->tcf_index,
+		.refcnt  = refcount_read(&ci->tcf_refcnt) - ref,
+		.bindcnt = atomic_read(&ci->tcf_bindcnt) - bind,
+	};
+	unsigned char *b = skb_tail_pointer(skb);
+	struct tcf_ctinfo_params *cp;
+	struct tcf_t t;
+
+	spin_lock_bh(&ci->tcf_lock);
+	cp = rcu_dereference_protected(ci->params,
+				       lockdep_is_held(&ci->tcf_lock));
+
+	tcf_tm_dump(&t, &ci->tcf_tm);
+	if (nla_put_64bit(skb, TCA_CTINFO_TM, sizeof(t), &t, TCA_CTINFO_PAD))
+		goto nla_put_failure;
+
+	opt.action = ci->tcf_action;
+	if (nla_put(skb, TCA_CTINFO_ACT, sizeof(opt), &opt))
+		goto nla_put_failure;
+
+	if (nla_put_u16(skb, TCA_CTINFO_ZONE, cp->zone))
+		goto nla_put_failure;
+
+	if (cp->mode & CTINFO_MODE_DSCP) {
+		if (nla_put_u32(skb, TCA_CTINFO_PARMS_DSCP_MASK,
+				cp->dscpmask))
+			goto nla_put_failure;
+		if (nla_put_u32(skb, TCA_CTINFO_PARMS_DSCP_STATEMASK,
+				cp->dscpstatemask))
+			goto nla_put_failure;
+	}
+
+	if (cp->mode & CTINFO_MODE_CPMARK) {
+		if (nla_put_u32(skb, TCA_CTINFO_PARMS_CPMARK_MASK,
+				cp->cpmarkmask))
+			goto nla_put_failure;
+	}
+
+	if (nla_put_u64_64bit(skb, TCA_CTINFO_STATS_DSCP_SET,
+			      ci->stats_dscp_set, TCA_CTINFO_PAD))
+		goto nla_put_failure;
+
+	if (nla_put_u64_64bit(skb, TCA_CTINFO_STATS_DSCP_ERROR,
+			      ci->stats_dscp_error, TCA_CTINFO_PAD))
+		goto nla_put_failure;
+
+	if (nla_put_u64_64bit(skb, TCA_CTINFO_STATS_CPMARK_SET,
+			      ci->stats_cpmark_set, TCA_CTINFO_PAD))
+		goto nla_put_failure;
+
+	spin_unlock_bh(&ci->tcf_lock);
+	return skb->len;
+
+nla_put_failure:
+	spin_unlock_bh(&ci->tcf_lock);
+	nlmsg_trim(skb, b);
+	return -1;
+}
+
+static int tcf_ctinfo_walker(struct net *net, struct sk_buff *skb,
+			     struct netlink_callback *cb, int type,
+			     const struct tc_action_ops *ops,
+			     struct netlink_ext_ack *extack)
+{
+	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
+
+	return tcf_generic_walker(tn, skb, cb, type, ops, extack);
+}
+
+static int tcf_ctinfo_search(struct net *net, struct tc_action **a, u32 index)
+{
+	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
+
+	return tcf_idr_search(tn, a, index);
+}
+
+static struct tc_action_ops act_ctinfo_ops = {
+	.kind	= "ctinfo",
+	.id	= TCA_ID_CTINFO,
+	.owner	= THIS_MODULE,
+	.act	= tcf_ctinfo_act,
+	.dump	= tcf_ctinfo_dump,
+	.init	= tcf_ctinfo_init,
+	.walk	= tcf_ctinfo_walker,
+	.lookup	= tcf_ctinfo_search,
+	.size	= sizeof(struct tcf_ctinfo),
+};
+
+static __net_init int ctinfo_init_net(struct net *net)
+{
+	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
+
+	return tc_action_net_init(tn, &act_ctinfo_ops);
+}
+
+static void __net_exit ctinfo_exit_net(struct list_head *net_list)
+{
+	tc_action_net_exit(net_list, ctinfo_net_id);
+}
+
+static struct pernet_operations ctinfo_net_ops = {
+	.init		= ctinfo_init_net,
+	.exit_batch	= ctinfo_exit_net,
+	.id		= &ctinfo_net_id,
+	.size		= sizeof(struct tc_action_net),
+};
+
+static int __init ctinfo_init_module(void)
+{
+	return tcf_register_action(&act_ctinfo_ops, &ctinfo_net_ops);
+}
+
+static void __exit ctinfo_cleanup_module(void)
+{
+	tcf_unregister_action(&act_ctinfo_ops, &ctinfo_net_ops);
+}
+
+module_init(ctinfo_init_module);
+module_exit(ctinfo_cleanup_module);
+MODULE_AUTHOR("Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>");
+MODULE_DESCRIPTION("Connection tracking mark actions");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/tc-testing/config b/tools/testing/selftests/tc-testing/config
index 203302065458..b235efd55367 100644
--- a/tools/testing/selftests/tc-testing/config
+++ b/tools/testing/selftests/tc-testing/config
@@ -38,6 +38,7 @@ CONFIG_NET_ACT_CSUM=m
 CONFIG_NET_ACT_VLAN=m
 CONFIG_NET_ACT_BPF=m
 CONFIG_NET_ACT_CONNMARK=m
+CONFIG_NET_ACT_CTINFO=m
 CONFIG_NET_ACT_SKBMOD=m
 CONFIG_NET_ACT_IFE=m
 CONFIG_NET_ACT_TUNNEL_KEY=m
-- 
2.11.0

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Toke Høiland-Jørgensen]

Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:

> ctinfo is a new tc filter action module.  It is designed to restore
> information contained in firewall conntrack marks to other packet fields
> and is typically used on packet ingress paths.  At present it has two
> independent sub-functions or operating modes, DSCP restoration mode &
> skb mark restoration mode.
>
> The DSCP restore mode:
>
> This mode copies DSCP values that have been placed in the firewall
> conntrack mark back into the IPv4/v6 diffserv fields of relevant
> packets.
>
> The DSCP restoration is intended for use and has been found useful for
> restoring ingress classifications based on egress classifications across
> links that bleach or otherwise change DSCP, typically home ISP Internet
> links.  Restoring DSCP on ingress on the WAN link allows qdiscs such as
> but by no means limited to CAKE to shape inbound packets according to
> policies that are easier to set & mark on egress.
>
> Ingress classification is traditionally a challenging task since
> iptables rules haven't yet run and tc filter/eBPF programs are pre-NAT
> lookups, hence are unable to see internal IPv4 addresses as used on the
> typical home masquerading gateway.  Thus marking the connection in some
> manner on egress for later restoration of classification on ingress is
> easier to implement.
>
> Parameters related to DSCP restore mode:
>
> dscpmask - a 32 bit mask of 6 contiguous bits and indicate bits of the
> conntrack mark field contain the DSCP value to be restored.
>
> statemask - a 32 bit mask of (usually) 1 bit length, outside the area
> specified by dscpmask.  This represents a conditional operation flag
> whereby the DSCP is only restored if the flag is set.  This is useful to
> implement a 'one shot' iptables based classification where the
> 'complicated' iptables rules are only run once to classify the
> connection on initial (egress) packet and subsequent packets are all
> marked/restored with the same DSCP.  A mask of zero disables the
> conditional behaviour ie. the conntrack mark DSCP bits are always
> restored to the ip diffserv field (assuming the conntrack entry is found
> & the skb is an ipv4/ipv6 type)
>
> e.g. dscpmask 0xfc000000 statemask 0x01000000
>
> |----0xFC----conntrack mark----000000---|
> | Bits 31-26 | bit 25 | bit24 |~~~ Bit 0|
> | DSCP       | unused | flag  |unused   |
> |-----------------------0x01---000000---|
>       |                   |
>       |                   |
>       ---|             Conditional flag
>          v             only restore if set
> |-ip diffserv-|
> | 6 bits      |
> |-------------|
>
> The skb mark restore mode (cpmark):
>
> This mode copies the firewall conntrack mark to the skb's mark field.
> It is completely the functional equivalent of the existing act_connmark
> action with the additional feature of being able to apply a mask to the
> restored value.
>
> Parameters related to skb mark restore mode:
>
> mask - a 32 bit mask applied to the firewall conntrack mark to mask out
> bits unwanted for restoration.  This can be useful where the conntrack
> mark is being used for different purposes by different applications.  If
> not specified and by default the whole mark field is copied (i.e.
> default mask of 0xffffffff)
>
> e.g. mask 0x00ffffff to mask out the top 8 bits being used by the
> aforementioned DSCP restore mode.
>
> |----0x00----conntrack mark----ffffff---|
> | Bits 31-24 |                          |
> | DSCP & flag|      some value here     |
> |---------------------------------------|
> 			|
> 			|
> 			v
> |------------skb mark-------------------|
> |            |                          |
> |  zeroed    |                          |
> |---------------------------------------|
>
> Overall parameters:
>
> zone - conntrack zone
>
> control - action related control (reclassify | pipe | drop | continue |
> ok | goto chain <CHAIN_INDEX>)
>
> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>

Thank you for doing another iteration!

No further comments on the actual code, but I still get the whitespace
issue with the patch... And now it results in stray ^M characters in the
Kconfig file, which makes the build blow up :/

Not sure why that happens, but there are quite a few settings in git
related to line endings, so I guess something is going wrong with how
those interact with your editor settings? This indicates that you may
just have to set core.autocrlf to true:
https://stackoverflow.com/a/16144559

Anyway, apart from the whitespace issue:

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Kevin 'ldir' Darbyshire-Bryant]

> On 28 May 2019, at 19:08, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
<stuff snipped>
> 
> Thank you for doing another iteration!
> 
> No further comments on the actual code, but I still get the whitespace
> issue with the patch... And now it results in stray ^M characters in the
> Kconfig file, which makes the build blow up :/

This is very odd.  I produced the last patch (v6) from within a debian VM
and sent it from there also.  No weird line endings in the locally produced
patch text and it applied cleanly to a local tree.  I’ve sent test patches
into the openwrt tree and applied those cleanly direct from patchwork.

Similarly I’ve downloaded the v5 patch from netdev patchwork http://patchwork.ozlabs.org/patch/1105755/mbox/ and applied that with git
am without problem.

Am totally confused!


Cheers,

Kevin D-B

gpg: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Toke Høiland-Jørgensen]

Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:

>> On 28 May 2019, at 19:08, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> 
> <stuff snipped>
>> 
>> Thank you for doing another iteration!
>> 
>> No further comments on the actual code, but I still get the whitespace
>> issue with the patch... And now it results in stray ^M characters in the
>> Kconfig file, which makes the build blow up :/
>
> This is very odd.  I produced the last patch (v6) from within a debian VM
> and sent it from there also.  No weird line endings in the locally produced
> patch text and it applied cleanly to a local tree.  I’ve sent test patches
> into the openwrt tree and applied those cleanly direct from patchwork.
>
> Similarly I’ve downloaded the v5 patch from netdev patchwork
> http://patchwork.ozlabs.org/patch/1105755/mbox/ and applied that with
> git am without problem.
>
> Am totally confused!

Hmm, yeah, that is odd. Guess it may be mangled somewhere in transit on
the way to my system? I'll try to investigate that...

Anyway, if I download the patch from patchwork and apply it manually
things seems to work, so I guess you can ignore this for now. I'm sure
Davem will let you know if he has problems applying the patch :)

-Toke

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Cong Wang]

On Tue, May 28, 2019 at 10:05 AM Kevin 'ldir' Darbyshire-Bryant
<ldir@darbyshire-bryant.me.uk> wrote:
>
> ctinfo is a new tc filter action module.  It is designed to restore
> information contained in firewall conntrack marks to other packet fields
> and is typically used on packet ingress paths.  At present it has two
> independent sub-functions or operating modes, DSCP restoration mode &
> skb mark restoration mode.

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks.

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[David Miller]

From: Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Tue, 28 May 2019 17:03:50 +0000

> ctinfo is a new tc filter action module.  It is designed to restore
> information contained in firewall conntrack marks to other packet fields
> and is typically used on packet ingress paths.  At present it has two
> independent sub-functions or operating modes, DSCP restoration mode &
> skb mark restoration mode.
 ...

Applied, thank you.

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Kevin 'ldir' Darbyshire-Bryant]

> On 30 May 2019, at 05:44, David Miller <davem@davemloft.net> wrote:
> 
> From: Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> Date: Tue, 28 May 2019 17:03:50 +0000
> 
>> ctinfo is a new tc filter action module.  It is designed to restore
>> information contained in firewall conntrack marks to other packet fields
>> and is typically used on packet ingress paths.  At present it has two
>> independent sub-functions or operating modes, DSCP restoration mode &
>> skb mark restoration mode.
> ...
> 
> Applied, thank you.

Thank you. Thanks to Toke & Cong also for their encouragement even if I
didn’t see it as that at the time.

Cheers,

Kevin

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Marcelo Ricardo Leitner]

On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
...
> +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> +			   struct nlattr *est, struct tc_action **a,
> +			   int ovr, int bind, bool rtnl_held,
> +			   struct tcf_proto *tp,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> +	struct tcf_ctinfo_params *cp_new;
> +	struct tcf_chain *goto_ch = NULL;
> +	u32 dscpmask = 0, dscpstatemask;
> +	struct tc_ctinfo *actparm;
> +	struct tcf_ctinfo *ci;
> +	u8 dscpmaskshift;
> +	int ret = 0, err;
> +
> +	if (!nla)
> +		return -EINVAL;
> +
> +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
                                                                       ^^^^
Hi, two things here:
Why not use the extack parameter here? Took me a while to notice
that the EINVAL was actually hiding the issue below.
And also on the other two EINVALs this function returns.


Seems there was a race when this code went in and the stricter check
added by
b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
8cb081746c03 ("netlink: make validation more configurable for future
strictness").

I can't add these actions with current net-next and iproute-next:
# ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
Error: NLA_F_NESTED is missing.
We have an error talking to the kernel

This also happens with the current post of act_ct and should also
happen with the act_mpls post (thus why Cc'ing John as well).

I'm not sure how we should fix this. In theory the kernel can't get
stricter with userspace here, as that breaks user applications as
above, so older actions can't use the more stricter parser. Should we
have some actions behaving one way, and newer ones in a different way?
That seems bad.

Or maybe all actions should just use nla_parse_nested_deprecated()?
I'm thinking this last. Yet, then the _deprecated suffix may not make
much sense here. WDYT?

Thanks,
Marcelo

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Jakub Kicinski]

On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
> On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> ...
> > +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> > +			   struct nlattr *est, struct tc_action **a,
> > +			   int ovr, int bind, bool rtnl_held,
> > +			   struct tcf_proto *tp,
> > +			   struct netlink_ext_ack *extack)
> > +{
> > +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> > +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> > +	struct tcf_ctinfo_params *cp_new;
> > +	struct tcf_chain *goto_ch = NULL;
> > +	u32 dscpmask = 0, dscpstatemask;
> > +	struct tc_ctinfo *actparm;
> > +	struct tcf_ctinfo *ci;
> > +	u8 dscpmaskshift;
> > +	int ret = 0, err;
> > +
> > +	if (!nla)
> > +		return -EINVAL;
> > +
> > +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);  
>                                                                        ^^^^
> Hi, two things here:
> Why not use the extack parameter here? Took me a while to notice
> that the EINVAL was actually hiding the issue below.
> And also on the other two EINVALs this function returns.
> 
> 
> Seems there was a race when this code went in and the stricter check
> added by
> b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
> 8cb081746c03 ("netlink: make validation more configurable for future
> strictness").
> 
> I can't add these actions with current net-next and iproute-next:
> # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> Error: NLA_F_NESTED is missing.
> We have an error talking to the kernel
> 
> This also happens with the current post of act_ct and should also
> happen with the act_mpls post (thus why Cc'ing John as well).
> 
> I'm not sure how we should fix this. In theory the kernel can't get
> stricter with userspace here, as that breaks user applications as
> above, so older actions can't use the more stricter parser. Should we
> have some actions behaving one way, and newer ones in a different way?
> That seems bad.
> 
> Or maybe all actions should just use nla_parse_nested_deprecated()?
> I'm thinking this last. Yet, then the _deprecated suffix may not make
> much sense here. WDYT?

Surely for new actions we can require strict validation, there is
no existing user space to speak of..  Perhaps act_ctinfo and act_ct
got slightly confused with the race you described, but in principle
there is nothing stopping new actions from implementing the user space
correctly, right?

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Marcelo Ricardo Leitner]

On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
> On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
> > On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> > ...
> > > +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> > > +			   struct nlattr *est, struct tc_action **a,
> > > +			   int ovr, int bind, bool rtnl_held,
> > > +			   struct tcf_proto *tp,
> > > +			   struct netlink_ext_ack *extack)
> > > +{
> > > +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> > > +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> > > +	struct tcf_ctinfo_params *cp_new;
> > > +	struct tcf_chain *goto_ch = NULL;
> > > +	u32 dscpmask = 0, dscpstatemask;
> > > +	struct tc_ctinfo *actparm;
> > > +	struct tcf_ctinfo *ci;
> > > +	u8 dscpmaskshift;
> > > +	int ret = 0, err;
> > > +
> > > +	if (!nla)
> > > +		return -EINVAL;
> > > +
> > > +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);  
> >                                                                        ^^^^
> > Hi, two things here:
> > Why not use the extack parameter here? Took me a while to notice
> > that the EINVAL was actually hiding the issue below.
> > And also on the other two EINVALs this function returns.
> > 
> > 
> > Seems there was a race when this code went in and the stricter check
> > added by
> > b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
> > 8cb081746c03 ("netlink: make validation more configurable for future
> > strictness").
> > 
> > I can't add these actions with current net-next and iproute-next:
> > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > Error: NLA_F_NESTED is missing.
> > We have an error talking to the kernel
> > 
> > This also happens with the current post of act_ct and should also
> > happen with the act_mpls post (thus why Cc'ing John as well).
> > 
> > I'm not sure how we should fix this. In theory the kernel can't get
> > stricter with userspace here, as that breaks user applications as
> > above, so older actions can't use the more stricter parser. Should we
> > have some actions behaving one way, and newer ones in a different way?
> > That seems bad.
> > 
> > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > much sense here. WDYT?
> 
> Surely for new actions we can require strict validation, there is
> no existing user space to speak of..  Perhaps act_ctinfo and act_ct

Other than the inconsistency amongst the actions, agreed.

> got slightly confused with the race you described, but in principle
> there is nothing stopping new actions from implementing the user space
> correctly, right?

AFAICT we need to patch iproute2 outside of the action code to cope
with it. Something like:

--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -214,7 +214,7 @@ done0:
                         tail = addattr_nest(n, MAX_MSG, ++prio);
                         addattr_l(n, MAX_MSG, TCA_ACT_KIND, k, strlen(k) + 1);

-                       ret = a->parse_aopt(a, &argc, &argv, TCA_ACT_OPTIONS,
+                       ret = a->parse_aopt(a, &argc, &argv, TCA_ACT_OPTIONS | NLA_F_NESTED,

This wouldn't break the older actions, yes, but then again, to expect
a different parsing behavior from different actions.. seems weird. :)

  Marcelo

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Johannes Berg]

(switching to my personal email)

> > I can't add these actions with current net-next and iproute-next:
> > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > Error: NLA_F_NESTED is missing.
> > We have an error talking to the kernel
> > 
> > This also happens with the current post of act_ct and should also
> > happen with the act_mpls post (thus why Cc'ing John as well).
> > 
> > I'm not sure how we should fix this. In theory the kernel can't get
> > stricter with userspace here, as that breaks user applications as
> > above, so older actions can't use the more stricter parser. Should we
> > have some actions behaving one way, and newer ones in a different way?
> > That seems bad.

I think you could just fix all of the actions in userspace, since the
older kernel would allow both with and without the flag, and then from a
userspace POV it all behaves the same, just the kernel accepts some
things without the flag for compatibility with older iproute2?

> > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > much sense here. WDYT?
> 
> Surely for new actions we can require strict validation, there is
> no existing user space to speak of..  

That was the original idea.

> Perhaps act_ctinfo and act_ct
> got slightly confused with the race you described, but in principle
> there is nothing stopping new actions from implementing the user space
> correctly, right?

There's one potential thing where you have a new command in netlink
(which thus will use strict validation), but you use existing code in
userspace to build the netlink message or parts thereof?

But then again you can just fix that while you test it, and the current
and older kernel will accept the stricter version for the existing use
of the existing code too, right?

johannes

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Michal Kubecek]

On Wed, Jun 12, 2019 at 08:56:10PM +0200, Johannes Berg wrote:
> (switching to my personal email)
> 
> > > I can't add these actions with current net-next and iproute-next:
> > > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > > Error: NLA_F_NESTED is missing.
> > > We have an error talking to the kernel
> > > 
> > > This also happens with the current post of act_ct and should also
> > > happen with the act_mpls post (thus why Cc'ing John as well).
> > > 
> > > I'm not sure how we should fix this. In theory the kernel can't get
> > > stricter with userspace here, as that breaks user applications as
> > > above, so older actions can't use the more stricter parser. Should we
> > > have some actions behaving one way, and newer ones in a different way?
> > > That seems bad.
> 
> I think you could just fix all of the actions in userspace, since the
> older kernel would allow both with and without the flag, and then from a
> userspace POV it all behaves the same, just the kernel accepts some
> things without the flag for compatibility with older iproute2?
> 
> > > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > > much sense here. WDYT?
> > 
> > Surely for new actions we can require strict validation, there is
> > no existing user space to speak of..  
> 
> That was the original idea.
> 
> > Perhaps act_ctinfo and act_ct
> > got slightly confused with the race you described, but in principle
> > there is nothing stopping new actions from implementing the user space
> > correctly, right?
> 
> There's one potential thing where you have a new command in netlink
> (which thus will use strict validation), but you use existing code in
> userspace to build the netlink message or parts thereof?
> 
> But then again you can just fix that while you test it, and the current
> and older kernel will accept the stricter version for the existing use
> of the existing code too, right?

Userspace can safely set NLA_F_NESTED on every nested attribute as there
are only few places in kernel where nla->type is accessed directly
rather than through nla_type() and those are rather specific (mostly
when attribute type is actually used as an array index). So the best
course of action would be letting userspace always set NLA_F_NESTED.
So kernel can only by strict on newly added attributes but userspace can
(and should) set NLA_F_NESTED always.

The opposite direction (kernel -> userspace) is more tricky as we can
never be sure there isn't some userspace client accessing the type directly
without masking out the flags. Thus kernel can only set NLA_F_NESTED on
new attributes where there cannot be any userspace program used to it
not being set.

Michal

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Jakub Kicinski]

On Wed, 12 Jun 2019 21:18:59 +0200, Michal Kubecek wrote:
> On Wed, Jun 12, 2019 at 08:56:10PM +0200, Johannes Berg wrote:
> > (switching to my personal email)
> >   
> > > > I can't add these actions with current net-next and iproute-next:
> > > > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > > > Error: NLA_F_NESTED is missing.
> > > > We have an error talking to the kernel
> > > > 
> > > > This also happens with the current post of act_ct and should also
> > > > happen with the act_mpls post (thus why Cc'ing John as well).
> > > > 
> > > > I'm not sure how we should fix this. In theory the kernel can't get
> > > > stricter with userspace here, as that breaks user applications as
> > > > above, so older actions can't use the more stricter parser. Should we
> > > > have some actions behaving one way, and newer ones in a different way?
> > > > That seems bad.  
> > 
> > I think you could just fix all of the actions in userspace, since the
> > older kernel would allow both with and without the flag, and then from a
> > userspace POV it all behaves the same, just the kernel accepts some
> > things without the flag for compatibility with older iproute2?
> >   
> > > > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > > > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > > > much sense here. WDYT?  
> > > 
> > > Surely for new actions we can require strict validation, there is
> > > no existing user space to speak of..    
> > 
> > That was the original idea.
> >   
> > > Perhaps act_ctinfo and act_ct
> > > got slightly confused with the race you described, but in principle
> > > there is nothing stopping new actions from implementing the user space
> > > correctly, right?  
> > 
> > There's one potential thing where you have a new command in netlink
> > (which thus will use strict validation), but you use existing code in
> > userspace to build the netlink message or parts thereof?
> > 
> > But then again you can just fix that while you test it, and the current
> > and older kernel will accept the stricter version for the existing use
> > of the existing code too, right?  
> 
> Userspace can safely set NLA_F_NESTED on every nested attribute as there
> are only few places in kernel where nla->type is accessed directly
> rather than through nla_type() and those are rather specific (mostly
> when attribute type is actually used as an array index). So the best
> course of action would be letting userspace always set NLA_F_NESTED.
> So kernel can only by strict on newly added attributes but userspace can
> (and should) set NLA_F_NESTED always.
> 
> The opposite direction (kernel -> userspace) is more tricky as we can
> never be sure there isn't some userspace client accessing the type directly
> without masking out the flags. Thus kernel can only set NLA_F_NESTED on
> new attributes where there cannot be any userspace program used to it
> not being set.

Agreed, so it's just the slight inconsistency in the dumps, which I'd
think is a fair price to pay here.  Old user space won't recognize the
new attributes, anyway, so doesn't matter what flags they have..

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Simon Horman]

On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
> On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
> > On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> > ...
> > > +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> > > +			   struct nlattr *est, struct tc_action **a,
> > > +			   int ovr, int bind, bool rtnl_held,
> > > +			   struct tcf_proto *tp,
> > > +			   struct netlink_ext_ack *extack)
> > > +{
> > > +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> > > +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> > > +	struct tcf_ctinfo_params *cp_new;
> > > +	struct tcf_chain *goto_ch = NULL;
> > > +	u32 dscpmask = 0, dscpstatemask;
> > > +	struct tc_ctinfo *actparm;
> > > +	struct tcf_ctinfo *ci;
> > > +	u8 dscpmaskshift;
> > > +	int ret = 0, err;
> > > +
> > > +	if (!nla)
> > > +		return -EINVAL;
> > > +
> > > +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);  
> >                                                                        ^^^^
> > Hi, two things here:
> > Why not use the extack parameter here? Took me a while to notice
> > that the EINVAL was actually hiding the issue below.
> > And also on the other two EINVALs this function returns.
> > 
> > 
> > Seems there was a race when this code went in and the stricter check
> > added by
> > b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
> > 8cb081746c03 ("netlink: make validation more configurable for future
> > strictness").
> > 
> > I can't add these actions with current net-next and iproute-next:
> > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > Error: NLA_F_NESTED is missing.
> > We have an error talking to the kernel
> > 
> > This also happens with the current post of act_ct and should also
> > happen with the act_mpls post (thus why Cc'ing John as well).
> > 
> > I'm not sure how we should fix this. In theory the kernel can't get
> > stricter with userspace here, as that breaks user applications as
> > above, so older actions can't use the more stricter parser. Should we
> > have some actions behaving one way, and newer ones in a different way?
> > That seems bad.
> > 
> > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > much sense here. WDYT?
> 
> Surely for new actions we can require strict validation, there is
> no existing user space to speak of..  Perhaps act_ctinfo and act_ct
> got slightly confused with the race you described, but in principle
> there is nothing stopping new actions from implementing the user space
> correctly, right?

FWIW, that is my thinking too.

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Kevin 'ldir' Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 5215 bytes --]



> On 13 Jun 2019, at 10:33, Simon Horman <simon.horman@netronome.com> wrote:
> 
> On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
>> On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
>>> On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
>>> ...
>>>> +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>>>> +			   struct nlattr *est, struct tc_action **a,
>>>> +			   int ovr, int bind, bool rtnl_held,
>>>> +			   struct tcf_proto *tp,
>>>> +			   struct netlink_ext_ack *extack)
>>>> +{
>>>> +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
>>>> +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
>>>> +	struct tcf_ctinfo_params *cp_new;
>>>> +	struct tcf_chain *goto_ch = NULL;
>>>> +	u32 dscpmask = 0, dscpstatemask;
>>>> +	struct tc_ctinfo *actparm;
>>>> +	struct tcf_ctinfo *ci;
>>>> +	u8 dscpmaskshift;
>>>> +	int ret = 0, err;
>>>> +
>>>> +	if (!nla)
>>>> +		return -EINVAL;
>>>> +
>>>> +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
>>>                                                                       ^^^^
>>> Hi, two things here:
>>> Why not use the extack parameter here? Took me a while to notice
>>> that the EINVAL was actually hiding the issue below.
>>> And also on the other two EINVALs this function returns.
>>> 
>>> 
>>> Seems there was a race when this code went in and the stricter check
>>> added by
>>> b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
>>> 8cb081746c03 ("netlink: make validation more configurable for future
>>> strictness").
>>> 
>>> I can't add these actions with current net-next and iproute-next:
>>> # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
>>> Error: NLA_F_NESTED is missing.
>>> We have an error talking to the kernel
>>> 
>>> This also happens with the current post of act_ct and should also
>>> happen with the act_mpls post (thus why Cc'ing John as well).
>>> 
>>> I'm not sure how we should fix this. In theory the kernel can't get
>>> stricter with userspace here, as that breaks user applications as
>>> above, so older actions can't use the more stricter parser. Should we
>>> have some actions behaving one way, and newer ones in a different way?
>>> That seems bad.
>>> 
>>> Or maybe all actions should just use nla_parse_nested_deprecated()?
>>> I'm thinking this last. Yet, then the _deprecated suffix may not make
>>> much sense here. WDYT?
>> 
>> Surely for new actions we can require strict validation, there is
>> no existing user space to speak of..  Perhaps act_ctinfo and act_ct
>> got slightly confused with the race you described, but in principle
>> there is nothing stopping new actions from implementing the user space
>> correctly, right?
> 
> FWIW, that is my thinking too.


Hi everyone,

Apologies that somehow I seem to have caused a bit of trouble.  If need be
and because act_ctinfo hasn’t yet actually been released anything could happen
to it, reverted if need be.  I’d like it to be done right, not that I know
what right is, the perils of inexperience and copy/pasting existing boilerplate
code.

Looking at other code I think I should have done something like:

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index e78b60e47c0f..4695aa76c0dc 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -168,7 +168,7 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
        if (!nla)
                return -EINVAL;

-       err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
+       err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
        if (err < 0)
                return err;

@@ -182,13 +182,19 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
                dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
                /* need contiguous 6 bit mask */
                dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
-               if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
+               if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
+                       NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CTINFO_PARMS_DSCP_MASK],
+                                       "dscp mask must be 6 contiguous bits");
                        return -EINVAL;
+               }
                dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
                        nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
                /* mask & statemask must not overlap */
-               if (dscpmask & dscpstatemask)
+               if (dscpmask & dscpstatemask) {
+                       NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CTINFO_PARMS_STATEMASK],
+                                       "dscp statemask must not overlap dscp mask");
                        return -EINVAL;
+               }
        }

        /* done the validation:now to the actual action allocation */

Warning: Not even compile tested!  Am I heading in the right direction?


Cheers,

Kevin D-B

gpg: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Kevin 'ldir' Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]



> On 13 Jun 2019, at 11:09, Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> wrote:
> 
> 
<snip?
> 
> Warning: Not even compile tested!  Am I heading in the right direction?
> 

I think this is even better.  Does it help?  Clues before I send as proper patch :-)

Subject: [PATCH] sched: act_ctinfo: use extack error reporting

Use extack error reporting mechanism in addition to returning -EINVAL

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 net/sched/act_ctinfo.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index e78b60e47c0f..5fc1de4d7cf8 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -165,15 +165,20 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 	u8 dscpmaskshift;
 	int ret = 0, err;

-	if (!nla)
+	if (!nla) {
+		NL_SET_ERR_MSG_MOD(extack, "ctinfo requires attributes to be passed");
 		return -EINVAL;
+	}

-	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
+	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
 	if (err < 0)
 		return err;

-	if (!tb[TCA_CTINFO_ACT])
+	if (!tb[TCA_CTINFO_ACT]) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Missing required TCA_CTINFO_ACT attribute");
 		return -EINVAL;
+	}
 	actparm = nla_data(tb[TCA_CTINFO_ACT]);

 	/* do some basic validation here before dynamically allocating things */
@@ -182,13 +187,21 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 		dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
 		/* need contiguous 6 bit mask */
 		dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
-		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
+		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_CTINFO_PARMS_DSCP_MASK],
+					    "dscp mask must be 6 contiguous bits");
 			return -EINVAL;
+		}
 		dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
 			nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
 		/* mask & statemask must not overlap */
-		if (dscpmask & dscpstatemask)
+		if (dscpmask & dscpstatemask) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_CTINFO_PARMS_STATEMASK],
+					    "dscp statemask must not overlap dscp mask");
 			return -EINVAL;
+		}
 	}

 	/* done the validation:now to the actual action allocation */
--
2.20.1 (Apple Git-117)


Your patience & help much appreciated.

Kevin

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Toke Høiland-Jørgensen]

Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:

>> On 13 Jun 2019, at 10:33, Simon Horman <simon.horman@netronome.com> wrote:
>> 
>> On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
>>> On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
>>>> On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
>>>> ...
>>>>> +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>>>>> +			   struct nlattr *est, struct tc_action **a,
>>>>> +			   int ovr, int bind, bool rtnl_held,
>>>>> +			   struct tcf_proto *tp,
>>>>> +			   struct netlink_ext_ack *extack)
>>>>> +{
>>>>> +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
>>>>> +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
>>>>> +	struct tcf_ctinfo_params *cp_new;
>>>>> +	struct tcf_chain *goto_ch = NULL;
>>>>> +	u32 dscpmask = 0, dscpstatemask;
>>>>> +	struct tc_ctinfo *actparm;
>>>>> +	struct tcf_ctinfo *ci;
>>>>> +	u8 dscpmaskshift;
>>>>> +	int ret = 0, err;
>>>>> +
>>>>> +	if (!nla)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
>>>>                                                                       ^^^^
>>>> Hi, two things here:
>>>> Why not use the extack parameter here? Took me a while to notice
>>>> that the EINVAL was actually hiding the issue below.
>>>> And also on the other two EINVALs this function returns.
>>>> 
>>>> 
>>>> Seems there was a race when this code went in and the stricter check
>>>> added by
>>>> b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
>>>> 8cb081746c03 ("netlink: make validation more configurable for future
>>>> strictness").
>>>> 
>>>> I can't add these actions with current net-next and iproute-next:
>>>> # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
>>>> Error: NLA_F_NESTED is missing.
>>>> We have an error talking to the kernel
>>>> 
>>>> This also happens with the current post of act_ct and should also
>>>> happen with the act_mpls post (thus why Cc'ing John as well).
>>>> 
>>>> I'm not sure how we should fix this. In theory the kernel can't get
>>>> stricter with userspace here, as that breaks user applications as
>>>> above, so older actions can't use the more stricter parser. Should we
>>>> have some actions behaving one way, and newer ones in a different way?
>>>> That seems bad.
>>>> 
>>>> Or maybe all actions should just use nla_parse_nested_deprecated()?
>>>> I'm thinking this last. Yet, then the _deprecated suffix may not make
>>>> much sense here. WDYT?
>>> 
>>> Surely for new actions we can require strict validation, there is
>>> no existing user space to speak of..  Perhaps act_ctinfo and act_ct
>>> got slightly confused with the race you described, but in principle
>>> there is nothing stopping new actions from implementing the user space
>>> correctly, right?
>> 
>> FWIW, that is my thinking too.
>
>
> Hi everyone,
>
> Apologies that somehow I seem to have caused a bit of trouble.  If need be
> and because act_ctinfo hasn’t yet actually been released anything could happen
> to it, reverted if need be.  I’d like it to be done right, not that I know
> what right is, the perils of inexperience and copy/pasting existing boilerplate
> code.
>
> Looking at other code I think I should have done something like:
>
> diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> index e78b60e47c0f..4695aa76c0dc 100644
> --- a/net/sched/act_ctinfo.c
> +++ b/net/sched/act_ctinfo.c
> @@ -168,7 +168,7 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>         if (!nla)
>                 return -EINVAL;
>
> -       err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
> +       err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
>         if (err < 0)
>                 return err;
>
> @@ -182,13 +182,19 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>                 dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
>                 /* need contiguous 6 bit mask */
>                 dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
> -               if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
> +               if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
> +                       NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CTINFO_PARMS_DSCP_MASK],
> +                                       "dscp mask must be 6 contiguous bits");
>                         return -EINVAL;
> +               }
>                 dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
>                         nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
>                 /* mask & statemask must not overlap */
> -               if (dscpmask & dscpstatemask)
> +               if (dscpmask & dscpstatemask) {
> +                       NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CTINFO_PARMS_STATEMASK],
> +                                       "dscp statemask must not overlap dscp mask");
>                         return -EINVAL;
> +               }
>         }
>
>         /* done the validation:now to the actual action allocation */
>
> Warning: Not even compile tested!  Am I heading in the right
> direction?

Yup! Sending a follow-up patch with an update like this would be an
excellent way to fix the issue :)

-Toke

[RFC PATCH net-next] sched: act_ctinfo: use extack error reporting

[Kevin Darbyshire-Bryant]

Use extack error reporting mechanism in addition to returning -EINVAL

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 net/sched/act_ctinfo.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index e78b60e47c0f..a7d3679d7e2e 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -165,15 +165,20 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 	u8 dscpmaskshift;
 	int ret = 0, err;
 
-	if (!nla)
+	if (!nla) {
+		NL_SET_ERR_MSG_MOD(extack, "ctinfo requires attributes to be passed");
 		return -EINVAL;
+	}
 
-	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
+	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
 	if (err < 0)
 		return err;
 
-	if (!tb[TCA_CTINFO_ACT])
+	if (!tb[TCA_CTINFO_ACT]) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Missing required TCA_CTINFO_ACT attribute");
 		return -EINVAL;
+	}
 	actparm = nla_data(tb[TCA_CTINFO_ACT]);
 
 	/* do some basic validation here before dynamically allocating things */
@@ -182,13 +187,21 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 		dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
 		/* need contiguous 6 bit mask */
 		dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
-		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
+		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_CTINFO_PARMS_DSCP_MASK],
+					    "dscp mask must be 6 contiguous bits");
 			return -EINVAL;
+		}
 		dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
 			nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
 		/* mask & statemask must not overlap */
-		if (dscpmask & dscpstatemask)
+		if (dscpmask & dscpstatemask) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_CTINFO_PARMS_DSCP_STATEMASK],
+					    "dscp statemask must not overlap dscp mask");
 			return -EINVAL;
+		}
 	}
 
 	/* done the validation:now to the actual action allocation */
-- 
2.20.1 (Apple Git-117)

Re: [RFC PATCH net-next] sched: act_ctinfo: use extack error reporting

[Marcelo Ricardo Leitner]

On Thu, Jun 13, 2019 at 01:18:52PM +0200, Kevin Darbyshire-Bryant wrote:
> Use extack error reporting mechanism in addition to returning -EINVAL
> 
> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>

Nice. LGTM!

> ---
>  net/sched/act_ctinfo.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> index e78b60e47c0f..a7d3679d7e2e 100644
> --- a/net/sched/act_ctinfo.c
> +++ b/net/sched/act_ctinfo.c
> @@ -165,15 +165,20 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>  	u8 dscpmaskshift;
>  	int ret = 0, err;
>  
> -	if (!nla)
> +	if (!nla) {
> +		NL_SET_ERR_MSG_MOD(extack, "ctinfo requires attributes to be passed");
>  		return -EINVAL;
> +	}
>  
> -	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
> +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
>  	if (err < 0)
>  		return err;
>  
> -	if (!tb[TCA_CTINFO_ACT])
> +	if (!tb[TCA_CTINFO_ACT]) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Missing required TCA_CTINFO_ACT attribute");
>  		return -EINVAL;
> +	}
>  	actparm = nla_data(tb[TCA_CTINFO_ACT]);
>  
>  	/* do some basic validation here before dynamically allocating things */
> @@ -182,13 +187,21 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>  		dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
>  		/* need contiguous 6 bit mask */
>  		dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
> -		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
> +		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
> +			NL_SET_ERR_MSG_ATTR(extack,
> +					    tb[TCA_CTINFO_PARMS_DSCP_MASK],
> +					    "dscp mask must be 6 contiguous bits");
>  			return -EINVAL;
> +		}
>  		dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
>  			nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
>  		/* mask & statemask must not overlap */
> -		if (dscpmask & dscpstatemask)
> +		if (dscpmask & dscpstatemask) {
> +			NL_SET_ERR_MSG_ATTR(extack,
> +					    tb[TCA_CTINFO_PARMS_DSCP_STATEMASK],
> +					    "dscp statemask must not overlap dscp mask");
>  			return -EINVAL;
> +		}
>  	}
>  
>  	/* done the validation:now to the actual action allocation */
> -- 
> 2.20.1 (Apple Git-117)
> 

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Marcelo Ricardo Leitner]

On Thu, Jun 13, 2019 at 09:09:47AM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> 
> 
> > On 13 Jun 2019, at 10:33, Simon Horman <simon.horman@netronome.com> wrote:
> > 
> > On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
> >> On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
> >>> On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> >>> ...
> >>>> +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> >>>> +			   struct nlattr *est, struct tc_action **a,
> >>>> +			   int ovr, int bind, bool rtnl_held,
> >>>> +			   struct tcf_proto *tp,
> >>>> +			   struct netlink_ext_ack *extack)
> >>>> +{
> >>>> +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> >>>> +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> >>>> +	struct tcf_ctinfo_params *cp_new;
> >>>> +	struct tcf_chain *goto_ch = NULL;
> >>>> +	u32 dscpmask = 0, dscpstatemask;
> >>>> +	struct tc_ctinfo *actparm;
> >>>> +	struct tcf_ctinfo *ci;
> >>>> +	u8 dscpmaskshift;
> >>>> +	int ret = 0, err;
> >>>> +
> >>>> +	if (!nla)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
> >>>                                                                       ^^^^
> >>> Hi, two things here:
> >>> Why not use the extack parameter here? Took me a while to notice
> >>> that the EINVAL was actually hiding the issue below.
> >>> And also on the other two EINVALs this function returns.
> >>> 
> >>> 
> >>> Seems there was a race when this code went in and the stricter check
> >>> added by
> >>> b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
> >>> 8cb081746c03 ("netlink: make validation more configurable for future
> >>> strictness").
> >>> 
> >>> I can't add these actions with current net-next and iproute-next:
> >>> # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> >>> Error: NLA_F_NESTED is missing.
> >>> We have an error talking to the kernel
> >>> 
> >>> This also happens with the current post of act_ct and should also
> >>> happen with the act_mpls post (thus why Cc'ing John as well).
> >>> 
> >>> I'm not sure how we should fix this. In theory the kernel can't get
> >>> stricter with userspace here, as that breaks user applications as
> >>> above, so older actions can't use the more stricter parser. Should we
> >>> have some actions behaving one way, and newer ones in a different way?
> >>> That seems bad.
> >>> 
> >>> Or maybe all actions should just use nla_parse_nested_deprecated()?
> >>> I'm thinking this last. Yet, then the _deprecated suffix may not make
> >>> much sense here. WDYT?
> >> 
> >> Surely for new actions we can require strict validation, there is
> >> no existing user space to speak of..  Perhaps act_ctinfo and act_ct
> >> got slightly confused with the race you described, but in principle
> >> there is nothing stopping new actions from implementing the user space
> >> correctly, right?
> > 
> > FWIW, that is my thinking too.
> 
> 
> Hi everyone,
> 
> Apologies that somehow I seem to have caused a bit of trouble.  If need be

No need to be. :-)

  Marcelo

Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Marcelo Ricardo Leitner]

On Wed, Jun 12, 2019 at 02:34:58PM -0700, Jakub Kicinski wrote:
> On Wed, 12 Jun 2019 21:18:59 +0200, Michal Kubecek wrote:
> > On Wed, Jun 12, 2019 at 08:56:10PM +0200, Johannes Berg wrote:
> > > (switching to my personal email)
> > >   
> > > > > I can't add these actions with current net-next and iproute-next:
> > > > > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > > > > Error: NLA_F_NESTED is missing.
> > > > > We have an error talking to the kernel
> > > > > 
> > > > > This also happens with the current post of act_ct and should also
> > > > > happen with the act_mpls post (thus why Cc'ing John as well).
> > > > > 
> > > > > I'm not sure how we should fix this. In theory the kernel can't get
> > > > > stricter with userspace here, as that breaks user applications as
> > > > > above, so older actions can't use the more stricter parser. Should we
> > > > > have some actions behaving one way, and newer ones in a different way?
> > > > > That seems bad.  
> > > 
> > > I think you could just fix all of the actions in userspace, since the
> > > older kernel would allow both with and without the flag, and then from a
> > > userspace POV it all behaves the same, just the kernel accepts some
> > > things without the flag for compatibility with older iproute2?
> > >   
> > > > > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > > > > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > > > > much sense here. WDYT?  
> > > > 
> > > > Surely for new actions we can require strict validation, there is
> > > > no existing user space to speak of..    
> > > 
> > > That was the original idea.
> > >   
> > > > Perhaps act_ctinfo and act_ct
> > > > got slightly confused with the race you described, but in principle
> > > > there is nothing stopping new actions from implementing the user space
> > > > correctly, right?  
> > > 
> > > There's one potential thing where you have a new command in netlink
> > > (which thus will use strict validation), but you use existing code in
> > > userspace to build the netlink message or parts thereof?
> > > 
> > > But then again you can just fix that while you test it, and the current
> > > and older kernel will accept the stricter version for the existing use
> > > of the existing code too, right?  
> > 
> > Userspace can safely set NLA_F_NESTED on every nested attribute as there
> > are only few places in kernel where nla->type is accessed directly
> > rather than through nla_type() and those are rather specific (mostly
> > when attribute type is actually used as an array index). So the best
> > course of action would be letting userspace always set NLA_F_NESTED.
> > So kernel can only by strict on newly added attributes but userspace can
> > (and should) set NLA_F_NESTED always.
> > 
> > The opposite direction (kernel -> userspace) is more tricky as we can
> > never be sure there isn't some userspace client accessing the type directly
> > without masking out the flags. Thus kernel can only set NLA_F_NESTED on
> > new attributes where there cannot be any userspace program used to it
> > not being set.
> 
> Agreed, so it's just the slight inconsistency in the dumps, which I'd
> think is a fair price to pay here.  Old user space won't recognize the
> new attributes, anyway, so doesn't matter what flags they have..

Thanks for your inputs. In summary, being able to do extra validations
for new actions is worth the inconsitency then.

  Marcelo

[PATCH net-next] sched: act_ctinfo: use extack error reporting

[Kevin Darbyshire-Bryant]

Use extack error reporting mechanism in addition to returning -EINVAL

NL_SET_ERR_* code shamelessy copy/paste/adjusted from act_pedit &
sch_cake and used as reference as to what I should have done in the
first place.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 net/sched/act_ctinfo.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index e78b60e47c0f..a7d3679d7e2e 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -165,15 +165,20 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 	u8 dscpmaskshift;
 	int ret = 0, err;
 
-	if (!nla)
+	if (!nla) {
+		NL_SET_ERR_MSG_MOD(extack, "ctinfo requires attributes to be passed");
 		return -EINVAL;
+	}
 
-	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
+	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
 	if (err < 0)
 		return err;
 
-	if (!tb[TCA_CTINFO_ACT])
+	if (!tb[TCA_CTINFO_ACT]) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Missing required TCA_CTINFO_ACT attribute");
 		return -EINVAL;
+	}
 	actparm = nla_data(tb[TCA_CTINFO_ACT]);
 
 	/* do some basic validation here before dynamically allocating things */
@@ -182,13 +187,21 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 		dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
 		/* need contiguous 6 bit mask */
 		dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
-		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
+		if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_CTINFO_PARMS_DSCP_MASK],
+					    "dscp mask must be 6 contiguous bits");
 			return -EINVAL;
+		}
 		dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
 			nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
 		/* mask & statemask must not overlap */
-		if (dscpmask & dscpstatemask)
+		if (dscpmask & dscpstatemask) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_CTINFO_PARMS_DSCP_STATEMASK],
+					    "dscp statemask must not overlap dscp mask");
 			return -EINVAL;
+		}
 	}
 
 	/* done the validation:now to the actual action allocation */
-- 
2.20.1 (Apple Git-117)

Re: [PATCH net-next] sched: act_ctinfo: use extack error reporting

[David Miller]

From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Fri, 14 Jun 2019 10:09:44 +0100

> Use extack error reporting mechanism in addition to returning -EINVAL
> 
> NL_SET_ERR_* code shamelessy copy/paste/adjusted from act_pedit &
> sch_cake and used as reference as to what I should have done in the
> first place.
> 
> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>

Applied.