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