Re: [PATCHv3 16/24] IPVS: Add/adjust Netfilter hook functions and helpers for v6

["Julius Volz" <juliusv@google.com>]

On Wed, Sep 3, 2008 at 7:44 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Sep 02, 2008 at 03:55:47PM +0200, Julius Volz wrote:
>> Add Netfilter hook functions or modify existing ones, if possible, to
>> process IPv6 packets. Some support functions are also added/modified for
>> this. ip_vs_nat_icmp_v6() was already added in the patch that added the v6
>> xmit functions, as it is called from one of them.
>
> Hi Julius,
>
> I was looking over this code a bit more closely to rebase someone
> elses patch (which I intend to post very soon). I noticed two
> minor things.
>
> 1) ipl doesn't seem to be needed in ip_vs_out_icmp() any more.
>   But actually I'm quite ok with it staying there because the
>   patch I was working on will need it.

ihl is still used for the length of the first IP header in the skb.
The struct "ip_vs_iphdr ciph" is only used later, for the embedded IP
header in the ICMP packet. It's needed because pp->conn_out_get()
expects a pointer to this struct as a parameter. I hope that makes
sense or did I miss something?

> 2) I wonder if iph could be initialised earleir in ip_vs_out_icmp_v6()

I'll answer this below:

> [snip]
>
>> @@ -727,11 +759,117 @@ static int ip_vs_out_icmp(struct sk_buff *skb, int *related)
>
>>       return verdict;
>>  }
>>
>> -static inline int is_tcp_reset(const struct sk_buff *skb)
>> +#ifdef CONFIG_IP_VS_IPV6
>> +static int ip_vs_out_icmp_v6(struct sk_buff *skb, int *related)
>> +{
>> +     struct ipv6hdr *iph;
>> +     struct icmp6hdr _icmph, *ic;
>> +     struct ipv6hdr  _ciph, *cih;    /* The ip header contained
>> +                                        within the ICMP */
>> +     struct ip_vs_iphdr ciph;
>> +     struct ip_vs_conn *cp;
>> +     struct ip_vs_protocol *pp;
>> +     unsigned int offset, verdict;
>> +
>> +     *related = 1;
>> +
>> +     /* reassemble IP fragments */
>> +     if (ipv6_hdr(skb)->nexthdr == IPPROTO_FRAGMENT) {
>> +             if (ip_vs_gather_frags_v6(skb, IP_DEFRAG_VS_OUT))
>> +                     return NF_STOLEN;
>> +     }
>> +
>> +     iph = ipv6_hdr(skb);
>
> Would it be worthwhile rearanging things so that iph is intialised
> before "/* reassemble IP fragments */" and used instead
> of ipv6_hdr(skb)->nexthdr in the if statement?

I followed the same structure as in the v4 version of ip_vs_out_icmp()
here. I think this might be needed because ip_vs_gather_frags{_v6}
changes the location of the IP header in the skb during reassembly?

>> +     offset = sizeof(struct ipv6hdr);
>> +     ic = skb_header_pointer(skb, offset, sizeof(_icmph), &_icmph);
>> +     if (ic == NULL)
>> +             return NF_DROP;
>> +
>> +     IP_VS_DBG(12, "Outgoing ICMPv6 (%d,%d) " NIP6_FMT "->" NIP6_FMT "\n",
>> +               ic->icmp6_type, ntohs(icmpv6_id(ic)),
>> +               NIP6(iph->saddr), NIP6(iph->daddr));
>
> [snip]
>
>



-- 
Julius Volz - Corporate Operations - SysOps

Google Switzerland GmbH - Identification No.: CH-020.4.028.116-1