[PATCH net-next 0/2] net: sched: Introduce em_policy ematch

[PATCH net-next 0/2] net: sched: Introduce em_policy ematch

[Eyal Birger]

From: Eyal Birger <eyal@metanetworks.com>

The following patchset introduces a new tc ematch for matching IPSec
traffic from a tc context.

This allows early classification as well as mirroning/redirecting IPSec
traffic based on decapsulation criteria.

The matching functionality is based on the netfilter xt_policy match, and
shares code and data structures.

Eyal Birger (2):
  net: netfilter: export xt_policy match_policy_in() as
    xt_policy_match_policy_in()
  net: sched: add xfrm policy ematch

 include/net/netfilter/xt_policy.h |  12 ++++
 include/uapi/linux/pkt_cls.h      |   3 +-
 net/netfilter/xt_policy.c         |  18 +++---
 net/sched/Kconfig                 |  10 ++++
 net/sched/Makefile                |   1 +
 net/sched/em_policy.c             | 117 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 9 deletions(-)
 create mode 100644 include/net/netfilter/xt_policy.h
 create mode 100644 net/sched/em_policy.c

-- 
2.7.4

[PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Eyal Birger]

From: Eyal Birger <eyal@metanetworks.com>

Expose this functionality so it could be usable from a tc classifier.

The rename of match_policy_out() is done for consistency though it is not
exported.

Signed-off-by: Eyal Birger <eyal@metanetworks.com>
---
 include/net/netfilter/xt_policy.h | 12 ++++++++++++
 net/netfilter/xt_policy.c         | 18 ++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)
 create mode 100644 include/net/netfilter/xt_policy.h

diff --git a/include/net/netfilter/xt_policy.h b/include/net/netfilter/xt_policy.h
new file mode 100644
index 0000000..99dcd57
--- /dev/null
+++ b/include/net/netfilter/xt_policy.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _XT_POLICY_INT_H
+#define _XT_POLICY_INT_H
+
+#include <linux/skbuff.h>
+#include <linux/netfilter/xt_policy.h>
+
+int xt_policy_match_policy_in(const struct sk_buff *skb,
+			      const struct xt_policy_info *info,
+			      unsigned short family);
+
+#endif /* _XT_POLICY_INT_H */
diff --git a/net/netfilter/xt_policy.c b/net/netfilter/xt_policy.c
index 5639fb0..4f9d0b1 100644
--- a/net/netfilter/xt_policy.c
+++ b/net/netfilter/xt_policy.c
@@ -16,6 +16,7 @@
 #include <linux/netfilter.h>
 #include <linux/netfilter/xt_policy.h>
 #include <linux/netfilter/x_tables.h>
+#include <net/netfilter/xt_policy.h>
 
 MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
 MODULE_DESCRIPTION("Xtables: IPsec policy match");
@@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
 	       MATCH(reqid, x->props.reqid);
 }
 
-static int
-match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
-		unsigned short family)
+int xt_policy_match_policy_in(const struct sk_buff *skb,
+			      const struct xt_policy_info *info,
+			      unsigned short family)
 {
 	const struct xt_policy_elem *e;
 	const struct sec_path *sp = skb->sp;
@@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
 
 	return strict ? 1 : 0;
 }
+EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);
 
-static int
-match_policy_out(const struct sk_buff *skb, const struct xt_policy_info *info,
-		 unsigned short family)
+static int xt_policy_match_policy_out(const struct sk_buff *skb,
+				      const struct xt_policy_info *info,
+				      unsigned short family)
 {
 	const struct xt_policy_elem *e;
 	const struct dst_entry *dst = skb_dst(skb);
@@ -117,9 +119,9 @@ policy_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	int ret;
 
 	if (info->flags & XT_POLICY_MATCH_IN)
-		ret = match_policy_in(skb, info, xt_family(par));
+		ret = xt_policy_match_policy_in(skb, info, xt_family(par));
 	else
-		ret = match_policy_out(skb, info, xt_family(par));
+		ret = xt_policy_match_policy_out(skb, info, xt_family(par));
 
 	if (ret < 0)
 		ret = info->flags & XT_POLICY_MATCH_NONE ? true : false;
-- 
2.7.4

[PATCH net-next 2/2] net: sched: add xfrm policy ematch

[Eyal Birger]

From: Eyal Birger <eyal@metanetworks.com>

Allows classification based on the incoming IPSec policy used during
decpsulation.

This allows similar matching capabilities to those provided by netfilter
xt_policy module, and uses the same data strcuture - but from a tc entry
point.

Signed-off-by: Eyal Birger <eyal@metanetworks.com>
---
 include/uapi/linux/pkt_cls.h |   3 +-
 net/sched/Kconfig            |  10 ++++
 net/sched/Makefile           |   1 +
 net/sched/em_policy.c        | 117 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 net/sched/em_policy.c

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 46c5066..963842c 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -555,7 +555,8 @@ enum {
 #define	TCF_EM_VLAN		6
 #define	TCF_EM_CANID		7
 #define	TCF_EM_IPSET		8
-#define	TCF_EM_MAX		8
+#define	TCF_EM_POLICY		9
+#define	TCF_EM_MAX		9
 
 enum {
 	TCF_EM_PROG_TC
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index c03d86a..0670f53 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -658,6 +658,16 @@ config NET_EMATCH_IPSET
 	  To compile this code as a module, choose M here: the
 	  module will be called em_ipset.
 
+config NET_EMATCH_POLICY
+	tristate "Policy"
+	depends on NET_EMATCH && NETFILTER_XT_MATCH_POLICY
+	---help---
+	  Say Y here if you want to be able to classify packets based on
+	  IPsec policy that was used during decapsulation
+
+	  To compile this code as a module, choose M here: the
+	  module will be called em_policy.
+
 config NET_CLS_ACT
 	bool "Actions"
 	select NET_CLS
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 5b63544..7ca02a1 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -75,3 +75,4 @@ obj-$(CONFIG_NET_EMATCH_META)	+= em_meta.o
 obj-$(CONFIG_NET_EMATCH_TEXT)	+= em_text.o
 obj-$(CONFIG_NET_EMATCH_CANID)	+= em_canid.o
 obj-$(CONFIG_NET_EMATCH_IPSET)	+= em_ipset.o
+obj-$(CONFIG_NET_EMATCH_POLICY)	+= em_policy.o
diff --git a/net/sched/em_policy.c b/net/sched/em_policy.c
new file mode 100644
index 0000000..94ef318
--- /dev/null
+++ b/net/sched/em_policy.c
@@ -0,0 +1,117 @@
+/*
+ * net/sched/em_policy.c IPSec Policy Ematch
+ *
+ * (c) 2018 Eyal Birger <eyal.birger@gmail.com>
+ *
+ * Parts taken from netfilter/xt_policy.h:
+ * Copyright (c) 2004,2005 Patrick McHardy, <kaber@trash.net>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/gfp.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/skbuff.h>
+#include <linux/netfilter.h>
+#include <linux/netfilter/xt_policy.h>
+#include <net/pkt_cls.h>
+#include <net/netfilter/xt_policy.h>
+
+static int em_policy_change(struct net *net, void *data, int data_len,
+			    struct tcf_ematch *em)
+{
+	const struct xt_policy_info *info = (const void *)data;
+	__u16 dir_flags;
+
+	if (data_len != sizeof(*info))
+		return -EINVAL;
+
+	if (info->len > XT_POLICY_MAX_ELEM) {
+		pr_info("too many policy elements\n");
+		return -EINVAL;
+	}
+
+	dir_flags = info->flags & (XT_POLICY_MATCH_IN | XT_POLICY_MATCH_OUT);
+	if (dir_flags != XT_POLICY_MATCH_IN) {
+		pr_info("Only incoming policy can be matched\n");
+		return -EINVAL;
+	}
+
+	em->datalen = sizeof(*info);
+	em->data = (unsigned long)kmemdup(data, em->datalen, GFP_KERNEL);
+	if (!em->data)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void em_policy_destroy(struct tcf_ematch *em)
+{
+	const struct xt_policy_info *info = (const void *)em->data;
+
+	if (!info)
+		return;
+
+	kfree((void *)em->data);
+}
+
+static int em_policy_match(struct sk_buff *skb, struct tcf_ematch *em,
+			   struct tcf_pkt_info *info)
+{
+	const struct xt_policy_info *pol = (const void *)em->data;
+	unsigned short pf;
+	int ret;
+
+	switch (tc_skb_protocol(skb)) {
+	case htons(ETH_P_IP):
+		pf = NFPROTO_IPV4;
+		break;
+	case htons(ETH_P_IPV6):
+		pf = NFPROTO_IPV6;
+		break;
+	default:
+		return false;
+	}
+
+	ret = xt_policy_match_policy_in(skb, pol, pf);
+	if (ret < 0)
+		ret = pol->flags & XT_POLICY_MATCH_NONE ? true : false;
+	else if (pol->flags & XT_POLICY_MATCH_NONE)
+		ret = false;
+
+	return ret;
+}
+
+static struct tcf_ematch_ops em_policy_ops = {
+	.kind	  = TCF_EM_POLICY,
+	.change	  = em_policy_change,
+	.destroy  = em_policy_destroy,
+	.match	  = em_policy_match,
+	.owner	  = THIS_MODULE,
+	.link	  = LIST_HEAD_INIT(em_policy_ops.link)
+};
+
+static int __init init_em_policy(void)
+{
+	return tcf_em_register(&em_policy_ops);
+}
+
+static void __exit exit_em_policy(void)
+{
+	tcf_em_unregister(&em_policy_ops);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Eyal Birger <eyal.birger@gmail.com>");
+MODULE_DESCRIPTION("TC extended match for IPSec policies");
+
+module_init(init_em_policy);
+module_exit(exit_em_policy);
+
+MODULE_ALIAS_TCF_EMATCH(TCF_EM_POLICY);
-- 
2.7.4

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Pablo Neira Ayuso]

On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:
> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
>  	       MATCH(reqid, x->props.reqid);
>  }
>  
> -static int
> -match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
> -		unsigned short family)
> +int xt_policy_match_policy_in(const struct sk_buff *skb,
> +			      const struct xt_policy_info *info,
> +			      unsigned short family)
>  {
>  	const struct xt_policy_elem *e;
>  	const struct sec_path *sp = skb->sp;
> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>  
>  	return strict ? 1 : 0;
>  }
> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);

If you just want to call xt_policy_match from tc, then you could use
tc ipt infrastructure instead.

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Eyal Birger]

On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:
>> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
>>              MATCH(reqid, x->props.reqid);
>>  }
>>
>> -static int
>> -match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>> -             unsigned short family)
>> +int xt_policy_match_policy_in(const struct sk_buff *skb,
>> +                           const struct xt_policy_info *info,
>> +                           unsigned short family)
>>  {
>>       const struct xt_policy_elem *e;
>>       const struct sec_path *sp = skb->sp;
>> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>>
>>       return strict ? 1 : 0;
>>  }
>> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);
>
> If you just want to call xt_policy_match from tc, then you could use
> tc ipt infrastructure instead.

Thanks for the suggestion -
Are you referring to act_ipt? it looks like it allows calling targets;
I couldn't
find a classifier calling a netfilter matcher.

Eyal.

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Pablo Neira Ayuso]

On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:
> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:
> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
> >>              MATCH(reqid, x->props.reqid);
> >>  }
> >>
> >> -static int
> >> -match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
> >> -             unsigned short family)
> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
> >> +                           const struct xt_policy_info *info,
> >> +                           unsigned short family)
> >>  {
> >>       const struct xt_policy_elem *e;
> >>       const struct sec_path *sp = skb->sp;
> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
> >>
> >>       return strict ? 1 : 0;
> >>  }
> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);
> >
> > If you just want to call xt_policy_match from tc, then you could use
> > tc ipt infrastructure instead.
> 
> Thanks for the suggestion -
> Are you referring to act_ipt? it looks like it allows calling targets;
> I couldn't find a classifier calling a netfilter matcher.

Then, I'd suggest you extend that infrastructure to alllow to call
matches, so we reduce the number of interdepencies between different
subsystems.

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Eyal Birger]

On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:
>> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:
>> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
>> >>              MATCH(reqid, x->props.reqid);
>> >>  }
>> >>
>> >> -static int
>> >> -match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>> >> -             unsigned short family)
>> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
>> >> +                           const struct xt_policy_info *info,
>> >> +                           unsigned short family)
>> >>  {
>> >>       const struct xt_policy_elem *e;
>> >>       const struct sec_path *sp = skb->sp;
>> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>> >>
>> >>       return strict ? 1 : 0;
>> >>  }
>> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);
>> >
>> > If you just want to call xt_policy_match from tc, then you could use
>> > tc ipt infrastructure instead.
>>
>> Thanks for the suggestion -
>> Are you referring to act_ipt? it looks like it allows calling targets;
>> I couldn't find a classifier calling a netfilter matcher.
>
> Then, I'd suggest you extend that infrastructure to alllow to call
> matches, so we reduce the number of interdepencies between different
> subsystems.

This appears very versatile. though in this case the use of the xtables code and
structures was done in order to avoid introducing new uapi structures
and supporting
match code, not necessarily to expose the full capabilities of extended matches,
similar in spirit to what was done in the em_ipset ematch.

Perhaps in order to avoid the direct export of xt_policy code, I could call
xt_request_find_match() from the em_policy module, requesting the
xt_policy match?
this way api exposure is minimized while not overly complicating the
scope of this feature.

What do you think?
Eyal.

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Pablo Neira Ayuso]

On Sun, Jan 14, 2018 at 02:47:46PM +0200, Eyal Birger wrote:
> On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:
> >> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:
> >> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
> >> >>              MATCH(reqid, x->props.reqid);
> >> >>  }
> >> >>
> >> >> -static int
> >> >> -match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
> >> >> -             unsigned short family)
> >> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
> >> >> +                           const struct xt_policy_info *info,
> >> >> +                           unsigned short family)
> >> >>  {
> >> >>       const struct xt_policy_elem *e;
> >> >>       const struct sec_path *sp = skb->sp;
> >> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
> >> >>
> >> >>       return strict ? 1 : 0;
> >> >>  }
> >> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);
> >> >
> >> > If you just want to call xt_policy_match from tc, then you could use
> >> > tc ipt infrastructure instead.
> >>
> >> Thanks for the suggestion -
> >> Are you referring to act_ipt? it looks like it allows calling targets;
> >> I couldn't find a classifier calling a netfilter matcher.
> >
> > Then, I'd suggest you extend that infrastructure to alllow to call
> > matches, so we reduce the number of interdepencies between different
> > subsystems.
> 
> This appears very versatile. though in this case the use of the xtables code and
> structures was done in order to avoid introducing new uapi structures
> and supporting
> match code, not necessarily to expose the full capabilities of extended matches,
> similar in spirit to what was done in the em_ipset ematch.
> 
> Perhaps in order to avoid the direct export of xt_policy code, I could call
> xt_request_find_match() from the em_policy module, requesting the
> xt_policy match?
> this way api exposure is minimized while not overly complicating the
> scope of this feature.
> 
> What do you think?

That would look better indeed.

But once you call xt_request_find_match() from there, how far is to
allow any arbitrary match? I think you only have to specify the match
name, family and the binary layout structure that represents
xt_policy, right?

I'm telling this, because I think it would be fair enough to me if you
add the generic infrastructure to the kernel to allow arbitrary load
of xt matches, and then from userspace you just add the code to
support this which is what you need.

Probably someone else - not you - may follow up later on to generalize
the userspace codebase to support other matches, by when that happens,
the right bits will be in the kernel already.

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Eyal Birger]

On Mon, Jan 15, 2018 at 12:57 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jan 14, 2018 at 02:47:46PM +0200, Eyal Birger wrote:
>> On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:
>> >> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:
>> >> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state *x, const struct xt_policy_elem *e,
>> >> >>              MATCH(reqid, x->props.reqid);
>> >> >>  }
>> >> >>
>> >> >> -static int
>> >> >> -match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>> >> >> -             unsigned short family)
>> >> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
>> >> >> +                           const struct xt_policy_info *info,
>> >> >> +                           unsigned short family)
>> >> >>  {
>> >> >>       const struct xt_policy_elem *e;
>> >> >>       const struct sec_path *sp = skb->sp;
>> >> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff *skb, const struct xt_policy_info *info,
>> >> >>
>> >> >>       return strict ? 1 : 0;
>> >> >>  }
>> >> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);
>> >> >
>> >> > If you just want to call xt_policy_match from tc, then you could use
>> >> > tc ipt infrastructure instead.
>> >>
>> >> Thanks for the suggestion -
>> >> Are you referring to act_ipt? it looks like it allows calling targets;
>> >> I couldn't find a classifier calling a netfilter matcher.
>> >
>> > Then, I'd suggest you extend that infrastructure to alllow to call
>> > matches, so we reduce the number of interdepencies between different
>> > subsystems.
>>
>> This appears very versatile. though in this case the use of the xtables code and
>> structures was done in order to avoid introducing new uapi structures
>> and supporting
>> match code, not necessarily to expose the full capabilities of extended matches,
>> similar in spirit to what was done in the em_ipset ematch.
>>
>> Perhaps in order to avoid the direct export of xt_policy code, I could call
>> xt_request_find_match() from the em_policy module, requesting the
>> xt_policy match?
>> this way api exposure is minimized while not overly complicating the
>> scope of this feature.
>>
>> What do you think?
>
> That would look better indeed.
>
> But once you call xt_request_find_match() from there, how far is to
> allow any arbitrary match? I think you only have to specify the match
> name, family and the binary layout structure that represents
> xt_policy, right?
>

I don't think that should be a problem. I'd need to pass the protocol onto
the ematches .change() callbacks and get the appropriate match from there.

> I'm telling this, because I think it would be fair enough to me if you
> add the generic infrastructure to the kernel to allow arbitrary load
> of xt matches, and then from userspace you just add the code to
> support this which is what you need.
>
> Probably someone else - not you - may follow up later on to generalize
> the userspace codebase to support other matches, by when that happens,
> the right bits will be in the kernel already.

I'm fine with submitting the more generic infrastructure.
Will follow up with a new series.

Thanks again!
Eyal.

Re: [PATCH net-next 2/2] net: sched: add xfrm policy ematch

[Cong Wang]

On Fri, Jan 12, 2018 at 4:57 AM, Eyal Birger <eyal.birger@gmail.com> wrote:
> +static void em_policy_destroy(struct tcf_ematch *em)
> +{
> +       const struct xt_policy_info *info = (const void *)em->data;
> +
> +       if (!info)
> +               return;
> +
> +       kfree((void *)em->data);
> +}

Nit: kfree() could handle NULL, no need to check.

Re: [PATCH net-next 2/2] net: sched: add xfrm policy ematch

[Eyal Birger]

On Tue, Jan 16, 2018 at 8:30 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Jan 12, 2018 at 4:57 AM, Eyal Birger <eyal.birger@gmail.com> wrote:
>> +static void em_policy_destroy(struct tcf_ematch *em)
>> +{
>> +       const struct xt_policy_info *info = (const void *)em->data;
>> +
>> +       if (!info)
>> +               return;
>> +
>> +       kfree((void *)em->data);
>> +}
>
> Nit: kfree() could handle NULL, no need to check.

Thanks Cong! I later realized I could use the default ematch destructor,
so this function could be removed entirely. However, as I plan to resubmit this
as a more generic ematch without a direct netfilter dependency, this
code will change significantly.

Thanks again,
Eyal.

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Eyal Birger]

Hi Pablo,

On Mon, 15 Jan 2018 13:48:41 +0200
Eyal Birger <eyal.birger@gmail.com> wrote:

> On Mon, Jan 15, 2018 at 12:57 PM, Pablo Neira Ayuso
> <pablo@netfilter.org> wrote:
> > On Sun, Jan 14, 2018 at 02:47:46PM +0200, Eyal Birger wrote:  
> >> On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso
> >> <pablo@netfilter.org> wrote:  
> >> > On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:  
> >> >> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso
> >> >> <pablo@netfilter.org> wrote:  
> >> >> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:  
> >> >> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state
> >> >> >> *x, const struct xt_policy_elem *e, MATCH(reqid,
> >> >> >> x->props.reqid); }
> >> >> >>
> >> >> >> -static int
> >> >> >> -match_policy_in(const struct sk_buff *skb, const struct
> >> >> >> xt_policy_info *info,
> >> >> >> -             unsigned short family)
> >> >> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
> >> >> >> +                           const struct xt_policy_info
> >> >> >> *info,
> >> >> >> +                           unsigned short family)
> >> >> >>  {
> >> >> >>       const struct xt_policy_elem *e;
> >> >> >>       const struct sec_path *sp = skb->sp;
> >> >> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff
> >> >> >> *skb, const struct xt_policy_info *info,
> >> >> >>
> >> >> >>       return strict ? 1 : 0;
> >> >> >>  }
> >> >> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);  
> >> >> >
> >> >> > If you just want to call xt_policy_match from tc, then you
> >> >> > could use tc ipt infrastructure instead.  
> >> >>
> >> >> Thanks for the suggestion -
> >> >> Are you referring to act_ipt? it looks like it allows calling
> >> >> targets; I couldn't find a classifier calling a netfilter
> >> >> matcher.  
> >> >
> >> > Then, I'd suggest you extend that infrastructure to alllow to
> >> > call matches, so we reduce the number of interdepencies between
> >> > different subsystems.  
> >>
> >> This appears very versatile. though in this case the use of the
> >> xtables code and structures was done in order to avoid introducing
> >> new uapi structures and supporting
> >> match code, not necessarily to expose the full capabilities of
> >> extended matches, similar in spirit to what was done in the
> >> em_ipset ematch.
> >>
> >> Perhaps in order to avoid the direct export of xt_policy code, I
> >> could call xt_request_find_match() from the em_policy module,
> >> requesting the xt_policy match?
> >> this way api exposure is minimized while not overly complicating
> >> the scope of this feature.
> >>
> >> What do you think?  
> >
> > That would look better indeed.
> >
> > But once you call xt_request_find_match() from there, how far is to
> > allow any arbitrary match? I think you only have to specify the
> > match name, family and the binary layout structure that represents
> > xt_policy, right?
> >  
> 
> I don't think that should be a problem. I'd need to pass the protocol
> onto the ematches .change() callbacks and get the appropriate match
> from there.
> 
> > I'm telling this, because I think it would be fair enough to me if
> > you add the generic infrastructure to the kernel to allow arbitrary
> > load of xt matches, and then from userspace you just add the code to
> > support this which is what you need.
> >
> > Probably someone else - not you - may follow up later on to
> > generalize the userspace codebase to support other matches, by when
> > that happens, the right bits will be in the kernel already.  
> 
> I'm fine with submitting the more generic infrastructure.
> Will follow up with a new series.

Following up on this thread, I think this feature would better be
implemented utilizing xt_policy from tc instead of supporting arbitrary
xt matches.

Feedback on the generic framework ([1], [2]) revolved around the ability
to create the skb environment for running matches accessing the
skb->data.

My concern is that it would be difficult to maintain the correct
environment for any xt match, whereas it is simple to create a
designated ematch for a specific xt match - as done for ipset - which
can validate the necessary prerequisites for that xt match.

It is also simple to dynamically fetch the xt_policy match function
using xt_request_find_match() as suggested in the em_ipt submittion.

I'd very much appreciate your feedback.

Thanks,
Eyal.

[1] https://patchwork.ozlabs.org/patch/864683/
[2] https://patchwork.ozlabs.org/patch/866490/

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Pablo Neira Ayuso]

On Wed, Feb 14, 2018 at 10:14:24AM +0200, Eyal Birger wrote:
> Hi Pablo,
> 
> On Mon, 15 Jan 2018 13:48:41 +0200
> Eyal Birger <eyal.birger@gmail.com> wrote:
> 
> > On Mon, Jan 15, 2018 at 12:57 PM, Pablo Neira Ayuso
> > <pablo@netfilter.org> wrote:
> > > On Sun, Jan 14, 2018 at 02:47:46PM +0200, Eyal Birger wrote:  
> > >> On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso
> > >> <pablo@netfilter.org> wrote:  
> > >> > On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:  
> > >> >> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso
> > >> >> <pablo@netfilter.org> wrote:  
> > >> >> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:  
> > >> >> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct xfrm_state
> > >> >> >> *x, const struct xt_policy_elem *e, MATCH(reqid,
> > >> >> >> x->props.reqid); }
> > >> >> >>
> > >> >> >> -static int
> > >> >> >> -match_policy_in(const struct sk_buff *skb, const struct
> > >> >> >> xt_policy_info *info,
> > >> >> >> -             unsigned short family)
> > >> >> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
> > >> >> >> +                           const struct xt_policy_info
> > >> >> >> *info,
> > >> >> >> +                           unsigned short family)
> > >> >> >>  {
> > >> >> >>       const struct xt_policy_elem *e;
> > >> >> >>       const struct sec_path *sp = skb->sp;
> > >> >> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff
> > >> >> >> *skb, const struct xt_policy_info *info,
> > >> >> >>
> > >> >> >>       return strict ? 1 : 0;
> > >> >> >>  }
> > >> >> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);  
> > >> >> >
> > >> >> > If you just want to call xt_policy_match from tc, then you
> > >> >> > could use tc ipt infrastructure instead.  
> > >> >>
> > >> >> Thanks for the suggestion -
> > >> >> Are you referring to act_ipt? it looks like it allows calling
> > >> >> targets; I couldn't find a classifier calling a netfilter
> > >> >> matcher.  
> > >> >
> > >> > Then, I'd suggest you extend that infrastructure to alllow to
> > >> > call matches, so we reduce the number of interdepencies between
> > >> > different subsystems.  
> > >>
> > >> This appears very versatile. though in this case the use of the
> > >> xtables code and structures was done in order to avoid introducing
> > >> new uapi structures and supporting
> > >> match code, not necessarily to expose the full capabilities of
> > >> extended matches, similar in spirit to what was done in the
> > >> em_ipset ematch.
> > >>
> > >> Perhaps in order to avoid the direct export of xt_policy code, I
> > >> could call xt_request_find_match() from the em_policy module,
> > >> requesting the xt_policy match?
> > >> this way api exposure is minimized while not overly complicating
> > >> the scope of this feature.
> > >>
> > >> What do you think?  
> > >
> > > That would look better indeed.
> > >
> > > But once you call xt_request_find_match() from there, how far is to
> > > allow any arbitrary match? I think you only have to specify the
> > > match name, family and the binary layout structure that represents
> > > xt_policy, right?
> > >  
> > 
> > I don't think that should be a problem. I'd need to pass the protocol
> > onto the ematches .change() callbacks and get the appropriate match
> > from there.
> > 
> > > I'm telling this, because I think it would be fair enough to me if
> > > you add the generic infrastructure to the kernel to allow arbitrary
> > > load of xt matches, and then from userspace you just add the code to
> > > support this which is what you need.
> > >
> > > Probably someone else - not you - may follow up later on to
> > > generalize the userspace codebase to support other matches, by when
> > > that happens, the right bits will be in the kernel already.  
> > 
> > I'm fine with submitting the more generic infrastructure.
> > Will follow up with a new series.
> 
> Following up on this thread, I think this feature would better be
> implemented utilizing xt_policy from tc instead of supporting arbitrary
> xt matches.
> 
> Feedback on the generic framework ([1], [2]) revolved around the ability
> to create the skb environment for running matches accessing the
> skb->data.

I think conclusion was that we're all fine. At ingress this turns into
noop and at egress there's no skb sharing at all. Anyway, see below.

> My concern is that it would be difficult to maintain the correct
> environment for any xt match, whereas it is simple to create a
> designated ematch for a specific xt match - as done for ipset - which
> can validate the necessary prerequisites for that xt match.

Then, artificially restrict this to work for xt_policy only. But please,
no new exported symbols to achieve this given you can do this with the
existing exported symbols. I mean no direct symbol dependencies with
xt_policy.

I'm fine if you just want to expose the policy match via tc, instead of
a generic ipt match infrastructure as long as you use the existing
exported symbols.

> It is also simple to dynamically fetch the xt_policy match function
> using xt_request_find_match() as suggested in the em_ipt submittion.

Exactly, you can use xt_request_find_match().

Thanks!

Re: [PATCH net-next 1/2] net: netfilter: export xt_policy match_policy_in() as xt_policy_match_policy_in()

[Eyal Birger]

Hi Pablo,

On Wed, 14 Feb 2018 11:19:40 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Wed, Feb 14, 2018 at 10:14:24AM +0200, Eyal Birger wrote:
> > Hi Pablo,
> > 
> > On Mon, 15 Jan 2018 13:48:41 +0200
> > Eyal Birger <eyal.birger@gmail.com> wrote:
> >   
> > > On Mon, Jan 15, 2018 at 12:57 PM, Pablo Neira Ayuso
> > > <pablo@netfilter.org> wrote:  
> > > > On Sun, Jan 14, 2018 at 02:47:46PM +0200, Eyal Birger wrote:    
> > > >> On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso
> > > >> <pablo@netfilter.org> wrote:    
> > > >> > On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger
> > > >> > wrote:    
> > > >> >> On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso
> > > >> >> <pablo@netfilter.org> wrote:    
> > > >> >> > On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger
> > > >> >> > wrote:    
> > > >> >> >> @@ -51,9 +52,9 @@ match_xfrm_state(const struct
> > > >> >> >> xfrm_state *x, const struct xt_policy_elem *e,
> > > >> >> >> MATCH(reqid, x->props.reqid); }
> > > >> >> >>
> > > >> >> >> -static int
> > > >> >> >> -match_policy_in(const struct sk_buff *skb, const struct
> > > >> >> >> xt_policy_info *info,
> > > >> >> >> -             unsigned short family)
> > > >> >> >> +int xt_policy_match_policy_in(const struct sk_buff *skb,
> > > >> >> >> +                           const struct xt_policy_info
> > > >> >> >> *info,
> > > >> >> >> +                           unsigned short family)
> > > >> >> >>  {
> > > >> >> >>       const struct xt_policy_elem *e;
> > > >> >> >>       const struct sec_path *sp = skb->sp;
> > > >> >> >> @@ -80,10 +81,11 @@ match_policy_in(const struct sk_buff
> > > >> >> >> *skb, const struct xt_policy_info *info,
> > > >> >> >>
> > > >> >> >>       return strict ? 1 : 0;
> > > >> >> >>  }
> > > >> >> >> +EXPORT_SYMBOL_GPL(xt_policy_match_policy_in);    
> > > >> >> >
> > > >> >> > If you just want to call xt_policy_match from tc, then you
> > > >> >> > could use tc ipt infrastructure instead.    
> > > >> >>
> > > >> >> Thanks for the suggestion -
> > > >> >> Are you referring to act_ipt? it looks like it allows
> > > >> >> calling targets; I couldn't find a classifier calling a
> > > >> >> netfilter matcher.    
> > > >> >
> > > >> > Then, I'd suggest you extend that infrastructure to alllow to
> > > >> > call matches, so we reduce the number of interdepencies
> > > >> > between different subsystems.    
> > > >>
> > > >> This appears very versatile. though in this case the use of the
> > > >> xtables code and structures was done in order to avoid
> > > >> introducing new uapi structures and supporting
> > > >> match code, not necessarily to expose the full capabilities of
> > > >> extended matches, similar in spirit to what was done in the
> > > >> em_ipset ematch.
> > > >>
> > > >> Perhaps in order to avoid the direct export of xt_policy code,
> > > >> I could call xt_request_find_match() from the em_policy module,
> > > >> requesting the xt_policy match?
> > > >> this way api exposure is minimized while not overly
> > > >> complicating the scope of this feature.
> > > >>
> > > >> What do you think?    
> > > >
> > > > That would look better indeed.
> > > >
> > > > But once you call xt_request_find_match() from there, how far
> > > > is to allow any arbitrary match? I think you only have to
> > > > specify the match name, family and the binary layout structure
> > > > that represents xt_policy, right?
> > > >    
> > > 
> > > I don't think that should be a problem. I'd need to pass the
> > > protocol onto the ematches .change() callbacks and get the
> > > appropriate match from there.
> > >   
> > > > I'm telling this, because I think it would be fair enough to me
> > > > if you add the generic infrastructure to the kernel to allow
> > > > arbitrary load of xt matches, and then from userspace you just
> > > > add the code to support this which is what you need.
> > > >
> > > > Probably someone else - not you - may follow up later on to
> > > > generalize the userspace codebase to support other matches, by
> > > > when that happens, the right bits will be in the kernel
> > > > already.    
> > > 
> > > I'm fine with submitting the more generic infrastructure.
> > > Will follow up with a new series.  
> > 
> > Following up on this thread, I think this feature would better be
> > implemented utilizing xt_policy from tc instead of supporting
> > arbitrary xt matches.
> > 
> > Feedback on the generic framework ([1], [2]) revolved around the
> > ability to create the skb environment for running matches accessing
> > the skb->data.  
> 
> I think conclusion was that we're all fine. At ingress this turns into
> noop and at egress there's no skb sharing at all. Anyway, see below.
> 
> > My concern is that it would be difficult to maintain the correct
> > environment for any xt match, whereas it is simple to create a
> > designated ematch for a specific xt match - as done for ipset -
> > which can validate the necessary prerequisites for that xt match.  
> 
> Then, artificially restrict this to work for xt_policy only. But
> please, no new exported symbols to achieve this given you can do this
> with the existing exported symbols. I mean no direct symbol
> dependencies with xt_policy.
> 
> I'm fine if you just want to expose the policy match via tc, instead
> of a generic ipt match infrastructure as long as you use the existing
> exported symbols.

New submitted version does not expose new netfilter symbols.

Thanks for your help!
Eyal.