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

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 6312 bytes --]

Aurélien Charbon wrote:
> Here is a small part of missing pieces of IPv6 support for the server.
> It deals with the ip_map caching code part.
> 
> It changes the ip_map structure to be able to store INET6 addresses.
> It adds also the changes in address hashing, and mapping to test it with 
> INET addresses.

Thanks for posting your patch.

Your strategy is to convert all incoming IPv4 addresses in the ip_map 
cache to IPv6 addresses, and use only IPv6 internally (often suggested 
by IPv6 books I've encountered).  For NFS, that is problematic because 
these addresses are used as the target of access control rules for 
exports; thus sys admins will expect to see IPv4 addresses in the output 
of NFS utilities if they specified IPv4 addresses in their /etc/exports 
file, for example.

Some naive questions:

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

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?  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.

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?

> 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?

> @@ -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?

> @@ -151,20 +155,28 @@
>  {
>      char text_addr[20];
>      struct ip_map *im = container_of(h, struct ip_map, h);
> -    __be32 addr = im->m_addr.s_addr;
> +   
> +    __be32 addr[4];
> +    int i;
> +    for (i=0;i<4;i++)
> +        addr[i] = im->m_addr.s6_addr[i];
>  
> -    snprintf(text_addr, 20, "%u.%u.%u.%u",
> -         ntohl(addr) >> 24 & 0xff,
> -         ntohl(addr) >> 16 & 0xff,
> -         ntohl(addr) >>  8 & 0xff,
> -         ntohl(addr) >>  0 & 0xff);
> +    snprintf(text_addr, 20, "%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
> +            ntohl(addr[3]) >> 16 & 0xff,
> +            ntohl(addr[3]) >>  0 & 0xff,
> +            ntohl(addr[2]) >> 16 & 0xff,
> +            ntohl(addr[2]) >>  0 & 0xff,
> +            ntohl(addr[1]) >> 16 & 0xff,
> +            ntohl(addr[1]) >>  0 & 0xff,
> +            ntohl(addr[0]) >> 16 & 0xff,
> +            ntohl(addr[0]) >>  0 & 0xff);

The snprintf() format strings should use NIP6_FMT.

> @@ -197,8 +209,21 @@
>      len = qword_get(&mesg, buf, mlen);
>      if (len <= 0) return -EINVAL;
>  
> -    if (sscanf(buf, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> -        return -EINVAL;
> +    if (sscanf(buf, "%d.%d.%d.%d%c", &b1, &b2, &b3, &b4, &c) == 4) {
> +               addr.s6_addr32[0] = 0;
> +               addr.s6_addr32[1] = 0;
> +               addr.s6_addr32[2] = htonl(0xffff);
> +               addr.s6_addr32[3] =
> +               htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> +       } else if (sscanf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x%c",
> +                         &b1, &b2, &b3, &b4, &b5, &b6, &b7, &b8, &c) == 
> 8) {
> +               addr.s6_addr32[0] = htonl((b1<<16)|b2);
> +               addr.s6_addr32[1] = htonl((b3<<16)|b4);
> +               addr.s6_addr32[2] = htonl((b5<<16)|b6);
> +               addr.s6_addr32[3] = htonl((b7<<16)|b8);
> +       } else
> +                return -EINVAL;
> +

Likewise, the sscanf() format strings should use NIP6_FMT.

> @@ -247,18 +269,22 @@
>      }
>      im = container_of(h, struct ip_map, h);
>      /* class addr domain */
> -    addr = im->m_addr;
> +    memcpy(&addr, &im->m_addr, sizeof(struct in6_addr));
>  
>      if (test_bit(CACHE_VALID, &h->flags) &&
>          !test_bit(CACHE_NEGATIVE, &h->flags))
>          dom = im->m_client->h.name;
>  
> -    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?

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 302 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel
version:2.1
end:vcard