Re: [PATCH 2/2] ebtables: Add string filter

[Pablo Neira Ayuso]

On Tue, Mar 13, 2018 at 01:32:18AM +, 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; kad...@blackhole.kfki.hu; 
> > f...@strlen.de; da...@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.har...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ebtables: Add string filter

[Bernie Harris]

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; kad...@blackhole.kfki.hu; 
> f...@strlen.de; da...@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.har...@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))
> >  

Re: [PATCH 2/2] ebtables: Add string filter

[Pablo Neira Ayuso]

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 
> ---
>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html