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

Reply via email to