Re: [PATCH net-next 0/2] netfilter: IPv4/v6 IPcomp match support

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Dec 19, 2013 at 11:30:09AM +0800, Fan Du wrote:
> 
> 
> On 2013年12月17日 21:05, Pablo Neira Ayuso wrote:
> >On Fri, Dec 13, 2013 at 08:18:00PM +0800, Fan Du wrote:
> >>Hi,
> >>
> >>This patchset adds IPv4/v6 IPComp 'match' plugin to enables user setting
> >>ACTONs for IPcomp flows sepecified with SPI value.
> >>
> >>Corresponding iptables patchset will be sent here after soon.
> >>
> >>Fan Du (2):
> >>   netfilter: add IPv4 IPComp extension match support
> >>   netfilter: add IPv6 IPComp extension match support
> >
> >This looks good, but I have to ask you to merge those two modules into
> >one single xt_ipcomp, they are fairly small and we can save the
> >overhead of having two different modules. Moreover, at quick glance I
> >don't see any dependency with IPv4/IPv6 exported symbols that may
> >cause ifdef pollution.
> >
> >Please, see net/netfilter/xt_tcpudp.c as reference to rework this.
> >Thanks.
> >
> 
> I noticed netfilter ipv4/v6 AH support also has split implementation,
> so far, by my understanding, it's fairly enough to consolidate those
> two implementations into one as well, as IPv4/6 AH head format are
> identical.
>
> If you don't mind or it won't break anything internal for netfilter,
> I plan to combine them into one piece.

We can merge those two, but you'll have to handle dependencies with
IPv6 core via ifdefs and Kconfig to avoid problems.

AH is not the last header, so we still have to use ipv6_find_hdr() to
find the good header instead of par->thoff. Note that the ip6_tables
sets par->thoff to the last IPv6 extension header.

This rises some concerns regarding your ipcomp, I think that if you
use this with ah and esp, the ordering of the headers is
ah+ipcomp+esp, right? In that case, you need to use ipv6_find_hdr()
since par->thoff would point to the esp header.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html