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

Hi Pablo,

On Wed, 14 Feb 2018 11:19:40 +0100
Pablo Neira Ayuso [off-list ref] wrote:

On Wed, Feb 14, 2018 at 10:14:24AM +0200, Eyal Birger wrote:

quoted

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.

quoted

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.