| Message ID | 20180226215835.7603-2-bernie.harris@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [1/2] net: Allow to and from offsets to be equal in skb_find_text | expand |
Hi Bernie, A few comments below. On Tue, Feb 27, 2018 at 10:58:35AM +1300, Bernie Harris wrote: > This patch is part of a proposal to add a string filter to > ebtables, which would be similar to the string filter in > iptables. > > Like iptables, the ebtables filter uses the xt_string module, > however some modifications have been made for this to work > correctly. > > Currently ebtables assumes that the revision number of all > match modules is 0. The xt_string module doesn't register a match > with revision 0 so the solution is to modify ebtables to allow > extensions to specify a revision number, similar to iptables. > This gets passed down to the kernel, which is then able to find > the match module correctly. > > Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz> > --- > include/uapi/linux/netfilter_bridge/ebtables.h | 5 ++++- > net/bridge/netfilter/ebtables.c | 12 ++++++++---- > net/netfilter/xt_string.c | 1 + > 3 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h > index 9ff57c0a0199..2143d5623d3b 100644 > --- a/include/uapi/linux/netfilter_bridge/ebtables.h > +++ b/include/uapi/linux/netfilter_bridge/ebtables.h > @@ -120,7 +120,10 @@ struct ebt_entries { > > struct ebt_entry_match { > union { > - char name[EBT_FUNCTION_MAXNAMELEN]; > + struct { > + char name[EBT_FUNCTION_MAXNAMELEN]; > + uint8_t revision; EBT_FUNCTION_MAXNAMELEN needs to be adjusted too to scratch this revision byte field. Otherwise, we break backward binary compatibility. > + }; > struct xt_match *match; > } u; > /* size of data */ > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c > index 02c4b409d317..6e55f3437fc8 100644 > --- a/net/bridge/netfilter/ebtables.c > +++ b/net/bridge/netfilter/ebtables.c > @@ -358,12 +358,12 @@ ebt_check_match(struct ebt_entry_match *m, struct xt_mtchk_param *par, > left - sizeof(struct ebt_entry_match) < m->match_size) > return -EINVAL; > > - match = xt_find_match(NFPROTO_BRIDGE, m->u.name, 0); > + match = xt_find_match(NFPROTO_BRIDGE, m->u.name, m->u.revision); > if (IS_ERR(match) || match->family != NFPROTO_BRIDGE) { > if (!IS_ERR(match)) > module_put(match->me); > request_module("ebt_%s", m->u.name); > - match = xt_find_match(NFPROTO_BRIDGE, m->u.name, 0); > + match = xt_find_match(NFPROTO_BRIDGE, m->u.name, m->u.revision); > } > if (IS_ERR(match)) > return PTR_ERR(match); > @@ -1604,7 +1604,10 @@ struct compat_ebt_replace { > /* struct ebt_entry_match, _target and _watcher have same layout */ > struct compat_ebt_entry_mwt { > union { > - char name[EBT_FUNCTION_MAXNAMELEN]; > + struct { > + char name[EBT_FUNCTION_MAXNAMELEN]; > + u8 revision; > + }; > compat_uptr_t ptr; > } u; > compat_uint_t match_size; > @@ -1948,7 +1951,8 @@ static int compat_mtw_from_user(struct compat_ebt_entry_mwt *mwt, > > switch (compat_mwt) { > case EBT_COMPAT_MATCH: > - match = xt_request_find_match(NFPROTO_BRIDGE, name, 0); > + match = xt_request_find_match(NFPROTO_BRIDGE, name, > + mwt->u.revision); > if (IS_ERR(match)) > return PTR_ERR(match); > Could you split this in two patches? One to add basic revision infrastructure to ebtables; and another one - oneliner patch containing the chunk below - to string matching support. Thanks! > diff --git a/net/netfilter/xt_string.c b/net/netfilter/xt_string.c > index 423293ee57c2..be1feddadcf0 100644 > --- a/net/netfilter/xt_string.c > +++ b/net/netfilter/xt_string.c > @@ -21,6 +21,7 @@ MODULE_DESCRIPTION("Xtables: string-based matching"); > MODULE_LICENSE("GPL"); > MODULE_ALIAS("ipt_string"); > MODULE_ALIAS("ip6t_string"); > +MODULE_ALIAS("ebt_string"); > > static bool > string_mt(const struct sk_buff *skb, struct xt_action_param *par) > -- > 2.16.1 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pablo, thanks for the reply. Just wanted to clarify your first comment below: On Mon, Mar 12, 2018 at 09:41:00AM +0100, Pablo Neira Ayuso wrote: > To: Bernie Harris > Cc: netfilter-devel@vger.kernel.org; kadlec@blackhole.kfki.hu; fw@strlen.de; davem@davemloft.net > Subject: Re: [PATCH 2/2] ebtables: Add string filter > > Hi Bernie, > > A few comments below. > > On Tue, Feb 27, 2018 at 10:58:35AM +1300, Bernie Harris wrote: > > This patch is part of a proposal to add a string filter to > > ebtables, which would be similar to the string filter in > > iptables. > > > > Like iptables, the ebtables filter uses the xt_string module, > > however some modifications have been made for this to work > > correctly. > > > > Currently ebtables assumes that the revision number of all > > match modules is 0. The xt_string module doesn't register a match > > with revision 0 so the solution is to modify ebtables to allow > > extensions to specify a revision number, similar to iptables. > > This gets passed down to the kernel, which is then able to find > > the match module correctly. > > > > Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz> > > --- > > include/uapi/linux/netfilter_bridge/ebtables.h | 5 ++++- > > net/bridge/netfilter/ebtables.c | 12 ++++++++---- > > net/netfilter/xt_string.c | 1 + > > 3 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h > > index 9ff57c0a0199..2143d5623d3b 100644 > > --- a/include/uapi/linux/netfilter_bridge/ebtables.h > > +++ b/include/uapi/linux/netfilter_bridge/ebtables.h > > @@ -120,7 +120,10 @@ struct ebt_entries { > > > > struct ebt_entry_match { > > union { > > - char name[EBT_FUNCTION_MAXNAMELEN]; > > + struct { > > + char name[EBT_FUNCTION_MAXNAMELEN]; > > + uint8_t revision; > > EBT_FUNCTION_MAXNAMELEN needs to be adjusted too to scratch this > revision byte field. Otherwise, we break backward binary > compatibility. > By this did you mean reduce EBT_FUNCTION_MAXNAMELEN by 1? Though I assume that would break a small number of existing setups. Alternatively, is there some way of adding a new ebt_entry_match_v2 structure that includes a revision field? Thanks > > + }; > > struct xt_match *match; > > } u; > > /* size of data */ > > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c > > index 02c4b409d317..6e55f3437fc8 100644 > > --- a/net/bridge/netfilter/ebtables.c > > +++ b/net/bridge/netfilter/ebtables.c > > @@ -358,12 +358,12 @@ ebt_check_match(struct ebt_entry_match *m, struct xt_mtchk_param *par, > > left - sizeof(struct ebt_entry_match) < m->match_size) > > return -EINVAL; > > > > - match = xt_find_match(NFPROTO_BRIDGE, m->u.name, 0); > > + match = xt_find_match(NFPROTO_BRIDGE, m->u.name, m->u.revision); > > if (IS_ERR(match) || match->family != NFPROTO_BRIDGE) { > > if (!IS_ERR(match)) > > module_put(match->me); > > request_module("ebt_%s", m->u.name); > > - match = xt_find_match(NFPROTO_BRIDGE, m->u.name, 0); > > + match = xt_find_match(NFPROTO_BRIDGE, m->u.name, m->u.revision); > > } > > if (IS_ERR(match)) > > return PTR_ERR(match); > > @@ -1604,7 +1604,10 @@ struct compat_ebt_replace { > > /* struct ebt_entry_match, _target and _watcher have same layout */ > > struct compat_ebt_entry_mwt { > > union { > > - char name[EBT_FUNCTION_MAXNAMELEN]; > > + struct { > > + char name[EBT_FUNCTION_MAXNAMELEN]; > > + u8 revision; > > + }; > > compat_uptr_t ptr; > > } u; > > compat_uint_t match_size; > > @@ -1948,7 +1951,8 @@ static int compat_mtw_from_user(struct compat_ebt_entry_mwt *mwt, > > > > switch (compat_mwt) { > > case EBT_COMPAT_MATCH: > > - match = xt_request_find_match(NFPROTO_BRIDGE, name, 0); > > + match = xt_request_find_match(NFPROTO_BRIDGE, name, > > + mwt->u.revision); > > if (IS_ERR(match)) > > return PTR_ERR(match); > > > > Could you split this in two patches? One to add basic revision > infrastructure to ebtables; and another one - oneliner patch > containing the chunk below - to string matching support. > > Thanks! > > > diff --git a/net/netfilter/xt_string.c b/net/netfilter/xt_string.c > > index 423293ee57c2..be1feddadcf0 100644 > > --- a/net/netfilter/xt_string.c > > +++ b/net/netfilter/xt_string.c > > @@ -21,6 +21,7 @@ MODULE_DESCRIPTION("Xtables: string-based matching"); > > MODULE_LICENSE("GPL"); > > MODULE_ALIAS("ipt_string"); > > MODULE_ALIAS("ip6t_string"); > > +MODULE_ALIAS("ebt_string"); > > > > static bool > > string_mt(const struct sk_buff *skb, struct xt_action_param *par) > > -- > > 2.16.1 > > > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 01:32:18AM +0000, Bernie Harris wrote: > Hi Pablo, thanks for the reply. Just wanted to clarify your first comment below: > > On Mon, Mar 12, 2018 at 09:41:00AM +0100, Pablo Neira Ayuso wrote: > > To: Bernie Harris > > Cc: netfilter-devel@vger.kernel.org; kadlec@blackhole.kfki.hu; fw@strlen.de; davem@davemloft.net > > Subject: Re: [PATCH 2/2] ebtables: Add string filter > > > > Hi Bernie, > > > > A few comments below. > > > > On Tue, Feb 27, 2018 at 10:58:35AM +1300, Bernie Harris wrote: > > > This patch is part of a proposal to add a string filter to > > > ebtables, which would be similar to the string filter in > > > iptables. > > > > > > Like iptables, the ebtables filter uses the xt_string module, > > > however some modifications have been made for this to work > > > correctly. > > > > > > Currently ebtables assumes that the revision number of all > > > match modules is 0. The xt_string module doesn't register a match > > > with revision 0 so the solution is to modify ebtables to allow > > > extensions to specify a revision number, similar to iptables. > > > This gets passed down to the kernel, which is then able to find > > > the match module correctly. > > > > > > Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz> > > > --- > > > include/uapi/linux/netfilter_bridge/ebtables.h | 5 ++++- > > > net/bridge/netfilter/ebtables.c | 12 ++++++++---- > > > net/netfilter/xt_string.c | 1 + > > > 3 files changed, 13 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h > > > index 9ff57c0a0199..2143d5623d3b 100644 > > > --- a/include/uapi/linux/netfilter_bridge/ebtables.h > > > +++ b/include/uapi/linux/netfilter_bridge/ebtables.h > > > @@ -120,7 +120,10 @@ struct ebt_entries { > > > > > > struct ebt_entry_match { > > > union { > > > - char name[EBT_FUNCTION_MAXNAMELEN]; > > > + struct { > > > + char name[EBT_FUNCTION_MAXNAMELEN]; > > > + uint8_t revision; > > > > EBT_FUNCTION_MAXNAMELEN needs to be adjusted too to scratch this > > revision byte field. Otherwise, we break backward binary > > compatibility. > > > > By this did you mean reduce EBT_FUNCTION_MAXNAMELEN by 1? Though I > assume that would break a small number of existing setups. Yes, we cannot update EBT_FUNCTION_MAXNAMELEN since other structures are using this too. Alternatively, we can add: #define EBT_EXTENSION_MAXNAMELEN 31 I think we'll end up seeing something like this: struct ebt_entry_match { union { struct { char name[EBT_EXTENSION_MAXNAMELEN]; __u8 revision; }; struct xt_match *match; } u; ... }; This is what we did in include/uapi/linux/netfilter/x_tables.h long long time ago to add support for match/target revisions, so this should be fine. I think there's a couple of chunks missing in your patch too, given that when we do copy_to_user() of the extension name. We have to place the revision there too so userspace knows what revision should be used for this. You don't likely need this now, but we may need this if there is a xt_string revision 2 in the future, so userspace knows what revision should be used. Moreover, the 32/64 compat code would need also an update - that code is there to allow userspace ebtables32 bits with 64bits kernels. > Alternatively, is there some way of adding a new ebt_entry_match_v2 > structure that includes a revision field? There is not, unfortunately. But we don't need this if we follow the approach I'm describing above. Let me know if there's still any concern on your side with this. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h index 9ff57c0a0199..2143d5623d3b 100644 --- a/include/uapi/linux/netfilter_bridge/ebtables.h +++ b/include/uapi/linux/netfilter_bridge/ebtables.h @@ -120,7 +120,10 @@ struct ebt_entries { struct ebt_entry_match { union { - char name[EBT_FUNCTION_MAXNAMELEN]; + struct { + char name[EBT_FUNCTION_MAXNAMELEN]; + uint8_t revision; + }; struct xt_match *match; } u; /* size of data */ diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 02c4b409d317..6e55f3437fc8 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -358,12 +358,12 @@ ebt_check_match(struct ebt_entry_match *m, struct xt_mtchk_param *par, left - sizeof(struct ebt_entry_match) < m->match_size) return -EINVAL; - match = xt_find_match(NFPROTO_BRIDGE, m->u.name, 0); + match = xt_find_match(NFPROTO_BRIDGE, m->u.name, m->u.revision); if (IS_ERR(match) || match->family != NFPROTO_BRIDGE) { if (!IS_ERR(match)) module_put(match->me); request_module("ebt_%s", m->u.name); - match = xt_find_match(NFPROTO_BRIDGE, m->u.name, 0); + match = xt_find_match(NFPROTO_BRIDGE, m->u.name, m->u.revision); } if (IS_ERR(match)) return PTR_ERR(match); @@ -1604,7 +1604,10 @@ struct compat_ebt_replace { /* struct ebt_entry_match, _target and _watcher have same layout */ struct compat_ebt_entry_mwt { union { - char name[EBT_FUNCTION_MAXNAMELEN]; + struct { + char name[EBT_FUNCTION_MAXNAMELEN]; + u8 revision; + }; compat_uptr_t ptr; } u; compat_uint_t match_size; @@ -1948,7 +1951,8 @@ static int compat_mtw_from_user(struct compat_ebt_entry_mwt *mwt, switch (compat_mwt) { case EBT_COMPAT_MATCH: - match = xt_request_find_match(NFPROTO_BRIDGE, name, 0); + match = xt_request_find_match(NFPROTO_BRIDGE, name, + mwt->u.revision); if (IS_ERR(match)) return PTR_ERR(match); diff --git a/net/netfilter/xt_string.c b/net/netfilter/xt_string.c index 423293ee57c2..be1feddadcf0 100644 --- a/net/netfilter/xt_string.c +++ b/net/netfilter/xt_string.c @@ -21,6 +21,7 @@ MODULE_DESCRIPTION("Xtables: string-based matching"); MODULE_LICENSE("GPL"); MODULE_ALIAS("ipt_string"); MODULE_ALIAS("ip6t_string"); +MODULE_ALIAS("ebt_string"); static bool string_mt(const struct sk_buff *skb, struct xt_action_param *par)
This patch is part of a proposal to add a string filter to ebtables, which would be similar to the string filter in iptables. Like iptables, the ebtables filter uses the xt_string module, however some modifications have been made for this to work correctly. Currently ebtables assumes that the revision number of all match modules is 0. The xt_string module doesn't register a match with revision 0 so the solution is to modify ebtables to allow extensions to specify a revision number, similar to iptables. This gets passed down to the kernel, which is then able to find the match module correctly. Signed-off-by: Bernie Harris <bernie.harris@alliedtelesis.co.nz> --- include/uapi/linux/netfilter_bridge/ebtables.h | 5 ++++- net/bridge/netfilter/ebtables.c | 12 ++++++++---- net/netfilter/xt_string.c | 1 + 3 files changed, 13 insertions(+), 5 deletions(-)