Date:   Wed, 14 Feb 2018 11:19:40 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org,
        coreteam@...filter.org, shmulik@...anetworks.com,
        Eyal Birger <eyal@...anetworks.com>, xiyou.wangcong@...il.com
Subject: 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 <eyal.birger@...il.com> wrote:
> 
> > On Mon, Jan 15, 2018 at 12:57 PM, Pablo Neira Ayuso
> > <pablo@...filter.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@...filter.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@...filter.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!

Powered by blists - more mailing lists