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

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 [off-list ref] wrote:

quoted

On Mon, Jan 15, 2018 at 12:57 PM, Pablo Neira Ayuso
[off-list ref] wrote:

quoted

On Sun, Jan 14, 2018 at 02:47:46PM +0200, Eyal Birger wrote:  

quoted

On Fri, Jan 12, 2018 at 4:00 PM, Pablo Neira Ayuso
[off-list ref] wrote:  

quoted

On Fri, Jan 12, 2018 at 03:56:21PM +0200, Eyal Birger wrote:  

quoted

On Fri, Jan 12, 2018 at 3:41 PM, Pablo Neira Ayuso
[off-list ref] wrote:  

quoted

On Fri, Jan 12, 2018 at 02:57:24PM +0200, Eyal Birger wrote:  

quoted

@@ -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.

quoted

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!