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

[Eyal Birger <eyal.birger@gmail.com>]

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/