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