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@xxxxxxxxxxxxxxx; kadlec@xxxxxxxxxxxxxxxxx; fw@xxxxxxxxx; davem@xxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
