| Message ID | 1515761845-31323-2-git-send-email-eyal.birger@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sched: Introduce em_policy ematch | expand |
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.
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.
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.
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.
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.
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.
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/
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!
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.
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;