Re: [PATCH 1/1] NFS: change the ip_map cache code to handle IPv6 addresses

["Aurélien Charbon" <aurelien.charbon@ext.bull.net>]

Thanks for these comments.

Chuck Lever wrote:

> Some naive questions:
>
> 1.  If IPv6 support is not configured into the kernel, how does an 
> IPv6-only cache work?

Yes it should work.
The INET6 structures are only used as containers for both type of address.
I have tested by unabling IPv6 options in the .config file.

>
> 2.  I seem to recall (only quite vaguely) that at some point the 
> server might need to use one of the stored addresses to, say, open a 
> socket to the client?  In that case, on a system with NICs configured 
> only with IPv4, is the cached IPv6 address properly converted back to 
> IPv4 somehow?

Yes. we can do that by testing the format of the address, as we know 
that an IPv4 address is mapped like that:
[00000000] [00000000] [0000FFFF] <32 bits of IPv4 addr>

> Can the cache code tell the difference between a cached IPv6 address 
> that was converted from IPv4 and one that was added to the cache as 
> IPv6?  Sorry I can't remember more clearly.

Yes. If the address starts with ::FFFF, we can infer that is a mapped 
IPv4 address.

>
> 3.  Would it be better to make the m_addr field a struct sockaddr, 
> store a whole address (with address family), and switch on the 
> sa_family field?

That is possible but we're able to retrieve the family of the address by 
testing its format (IPv4 mapped or not).
But I don't know if it is a better solution.

>
>
>> diff -u -r -N linux-2.6.23-rc1/fs/nfsd/export.c 
>> linux-2.6.23-rc1-IPv6-ip_map/fs/nfsd/export.c
>> --- linux-2.6.23-rc1/fs/nfsd/export.c    2007-08-08 
>> 17:52:58.000000000 +0200
>> +++ linux-2.6.23-rc1-IPv6-ip_map/fs/nfsd/export.c    2007-08-08 
>> 17:49:09.000000000 +0200
>> @@ -1558,6 +1558,7 @@
>>  {
>>      struct auth_domain    *dom;
>>      int            i, err;
>> +    struct in6_addr addr6;
>>  
>>      /* First, consistency check. */
>>      err = -EINVAL;
>> @@ -1576,9 +1577,14 @@
>>          goto out_unlock;
>>  
>>      /* Insert client into hashtable. */
>> -    for (i = 0; i < ncp->cl_naddr; i++)
>> -        auth_unix_add_addr(ncp->cl_addrlist[i], dom);
>> -
>> +    for (i = 0; i < ncp->cl_naddr; i++) {
>> +        /* Mapping address */
>> +            addr6.s6_addr32[0] = 0;
>> +            addr6.s6_addr32[1] = 0;
>> +            addr6.s6_addr32[2] = htonl(0xffff);
>> +            addr6.s6_addr32[3] = (uint32_t)ncp->cl_addrlist[i].s_addr;
>> +        auth_unix_add_addr(addr6, dom);
>> +    }
>>      auth_unix_forget_old(dom);
>>      auth_domain_put(dom);
>
>
> This converts IPv4 addresses to canonical IPv6 as it stores them.  
> What happens if a full-blown IPv6 address is encountered?  Likewise, 
> in nfsctl.c?

I have done this kind of convertion only for compilation an testing with 
only IPv4... For the moment.

>
>> @@ -112,12 +112,16 @@
>>      return (hash ^ (hash>>8)) & 0xff;
>>  }
>>  #endif
>> +static inline int hash_ip6(struct in6_addr ip)
>> +{
>> +        return (hash_ip(ip.s6_addr32[0]) ^ hash_ip(ip.s6_addr32[1]) 
>> ^ hash_ip(ip.s6_addr32[2]) ^ hash_ip(ip.s6_addr32[3])) ;
>> +}
>
>
> How have you tested the effectiveness of the new hash function?

I have not tested that point but I can easily imagine there are better 
solutions.
Perhaps we can keep the same function for an IPv4 address (only taking 
the 32 bits of IPv4 addr), and then design one for IPv6 addresses.

Do you have any suggestion on that ?

>> -    seq_printf(m, "%s %d.%d.%d.%d %s\n",
>> +    seq_printf(m, "%s %04x.%04x.%04x.%04x.%04x.%04x.%04x.%04x %s\n",
>>             im->m_class,
>> -           ntohl(addr.s_addr) >> 24 & 0xff,
>> -           ntohl(addr.s_addr) >> 16 & 0xff,
>> -           ntohl(addr.s_addr) >>  8 & 0xff,
>> -           ntohl(addr.s_addr) >>  0 & 0xff,
>> +           ntohl(addr.s6_addr32[3]) >> 16 & 0xffff,
>> +           ntohl(addr.s6_addr32[3]) & 0xffff,
>> +           ntohl(addr.s6_addr32[2]) >> 16 & 0xffff,
>> +           ntohl(addr.s6_addr32[2]) & 0xffff,
>> +           ntohl(addr.s6_addr32[1]) >> 16 & 0xffff,
>> +           ntohl(addr.s6_addr32[1]) & 0xffff,
>> +           ntohl(addr.s6_addr32[0]) >> 16 & 0xffff,
>> +           ntohl(addr.s6_addr32[0]) & 0xffff,
>>             dom
>>             );
>>      return 0;
>
>
> And I think here NIP6_FMT should be used, but you're not using colons 
> between the hex digits.  Was that intentional
> ?

No that's a mistake. Colons have to be used here.

Aurélien

-- 

********************************
       Aurelien Charbon
       Linux NFSv4 team
           Bull SAS
     Echirolles - France
http://nfsv4.bullopensource.org/
********************************