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

Message ID 1515761845-31323-2-git-send-email-eyal.birger@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • net: sched: Introduce em_policy ematch
Related show

Commit Message

Eyal Birger Jan. 12, 2018, 12:57 p.m.
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

Comments

Pablo Neira Ayuso Jan. 12, 2018, 1:41 p.m. | #1
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.
Eyal Birger Jan. 12, 2018, 1:56 p.m. | #2
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.
Pablo Neira Ayuso Jan. 12, 2018, 2 p.m. | #3
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.
Eyal Birger Jan. 14, 2018, 12:47 p.m. | #4
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.
Pablo Neira Ayuso Jan. 15, 2018, 10:57 a.m. | #5
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.
Eyal Birger Jan. 15, 2018, 11:48 a.m. | #6
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.
Eyal Birger Feb. 14, 2018, 8:14 a.m. | #7
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/
Pablo Neira Ayuso Feb. 14, 2018, 10:19 a.m. | #8
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!
Eyal Birger Feb. 15, 2018, 5:47 p.m. | #9
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.

Patch

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;