On Sun, Mar 11, 2018 at 11:04:22PM +0100, Matthias Schiffer wrote:
> On 03/11/2018 10:14 PM, Pablo Neira Ayuso wrote:
> > On Sun, Mar 04, 2018 at 09:28:53AM +0100, Matthias Schiffer wrote:
> >> We already have ICMPv6 type/code matches. This adds support for IPv4 ICMP
> >> matches in the same way.
> >>
> >> Signed-off-by: Matthias Schiffer <mschiffer@xxxxxxxxxxxxxxxxxxxx>
> >> ---
> >> include/uapi/linux/netfilter_bridge/ebt_ip.h | 13 +++++++--
> >> net/bridge/netfilter/ebt_ip.c | 43 +++++++++++++++++++++-------
> >> 2 files changed, 43 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/netfilter_bridge/ebt_ip.h b/include/uapi/linux/netfilter_bridge/ebt_ip.h
> >> index 8e462fb1983f..4ed7fbb0a482 100644
> >> --- a/include/uapi/linux/netfilter_bridge/ebt_ip.h
> >> +++ b/include/uapi/linux/netfilter_bridge/ebt_ip.h
> >> @@ -24,8 +24,9 @@
> >> #define EBT_IP_PROTO 0x08
> >> #define EBT_IP_SPORT 0x10
> >> #define EBT_IP_DPORT 0x20
> >> +#define EBT_IP_ICMP 0x40
> >> #define EBT_IP_MASK (EBT_IP_SOURCE | EBT_IP_DEST | EBT_IP_TOS | EBT_IP_PROTO |\
> >> - EBT_IP_SPORT | EBT_IP_DPORT )
> >> + EBT_IP_SPORT | EBT_IP_DPORT | EBT_IP_ICMP)
> >> #define EBT_IP_MATCH "ip"
> >>
> >> /* the same values are used for the invflags */
> >> @@ -38,8 +39,14 @@ struct ebt_ip_info {
> >> __u8 protocol;
> >> __u8 bitmask;
> >> __u8 invflags;
> >> - __u16 sport[2];
> >> - __u16 dport[2];
> >> + union {
> >> + __u16 sport[2];
> >> + __u8 icmp_type[2];
> >> + };
> >> + union {
> >> + __u16 dport[2];
> >> + __u8 icmp_code[2];
> >> + };
> >
> > This is part of uapi, we cannot update struct ebt_ip_info, this break
> > binary compatibility.
>
> I carefully extended the struct in a way that does not change API or ABI,
> in the same way the corresponding ICMPv6 matches were added in 6faee60a4e
> "netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes".
I see, thanks for explaining.
> >> };
> >>
> >> #endif
> >> diff --git a/net/bridge/netfilter/ebt_ip.c b/net/bridge/netfilter/ebt_ip.c
> >> index 2b46c50abce0..8cb8f8395768 100644
> >> --- a/net/bridge/netfilter/ebt_ip.c
> >> +++ b/net/bridge/netfilter/ebt_ip.c
> >
> > Please, place these new matching features into
> > net/bridge/netfilter/ebt_ip.c, please add then new ebt_xyz.c extension
> > instead.
>
> This would increase asymmetry between ebt_ip and ebt_ip6, rather than
> giving ebt_ip the same features that ebt_ip6 already had.
By looking at 6faee60a4e, you're right indeed.
> Is there any good reason to make it a new extension, when there is
> no problem to extend ebt_ip without changing UAPI/ABI?
I would suggest you place the IGMP match in a separated extension, so
we don't keep stuffing more code into ebt_ip which is rather going to
be called by everyone from the hotpath. I would expect few people are
going to need this, so I'd rather like to see this in the periphery.
--
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
