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

[Brian Haley <brian.haley@hp.com>]

Hi Aurelien,

Aurélien Charbon wrote:
> According to Neil's comments, I have tried to correct the mistakes of my 
> first sending

I have some more comments.

> @@ -1559,6 +1560,7 @@ exp_addclient(struct nfsctl_client *ncp)
> {
>     struct auth_domain    *dom;
>     int            i, err;
> +    struct in6_addr addr6;

Indentation looks wrong.

> diff -p -u -r -N linux-2.6.23-rc3/fs/nfsd/nfsctl.c 
> linux-2.6.23-rc3-IPv6-ipmap-cache/fs/nfsd/nfsctl.c
> --- linux-2.6.23-rc3/fs/nfsd/nfsctl.c    2007-08-23 13:18:16.000000000 
> +0200
> +++ linux-2.6.23-rc3-IPv6-ipmap-cache/fs/nfsd/nfsctl.c    2007-08-23 
> 13:25:28.000000000 +0200
> @@ -222,7 +222,7 @@ static ssize_t write_getfs(struct file *
>     struct auth_domain *clp;
>     int err = 0;
>     struct knfsd_fh *res;
> -
> +    struct in6_addr in6;

Indentation.

>     if (size < sizeof(*data))
>         return -EINVAL;
>     data = (struct nfsctl_fsparm*)buf;
> @@ -236,7 +236,14 @@ static ssize_t write_getfs(struct file *
>     res = (struct knfsd_fh*)buf;
> 
>     exp_readlock();
> -    if (!(clp = auth_unix_lookup(sin->sin_addr)))
> +
> +    /* IPv6 address mapping */
> +    in6.s6_addr32[0] = 0;
> +    in6.s6_addr32[1] = 0;
> +    in6.s6_addr32[2] = htonl(0xffff);
> +    in6.s6_addr32[3] = (uint32_t)sin->sin_addr.s_addr;

Why didn't you use your new ipv6_addr_map() inline here?

> @@ -253,6 +260,7 @@ static ssize_t write_getfd(struct file *
> {
>     struct nfsctl_fdparm *data;
>     struct sockaddr_in *sin;
> +    struct in6_addr in6;

Indentation.

> @@ -271,7 +279,14 @@ static ssize_t write_getfd(struct file *
>     res = buf;
>     sin = (struct sockaddr_in *)&data->gd_addr;
>     exp_readlock();
> -    if (!(clp = auth_unix_lookup(sin->sin_addr)))
> +
> +    /* IPv6 address mapping */
> +    in6.s6_addr32[0] = 0;
> +    in6.s6_addr32[1] = 0;
> +    in6.s6_addr32[2] = htonl(0xffff);
> +    in6.s6_addr32[3] = (uint32_t)sin->sin_addr.s_addr;

Why didn't you use your new ipv6_addr_map() inline here too?

> diff -p -u -r -N linux-2.6.23-rc3/include/net/ipv6.h 
> linux-2.6.23-rc3-IPv6-ipmap-cache/include/net/ipv6.h
> --- linux-2.6.23-rc3/include/net/ipv6.h    2007-08-23 13:18:23.000000000 
> +0200
> +++ linux-2.6.23-rc3-IPv6-ipmap-cache/include/net/ipv6.h    2007-08-23 
> 13:25:28.000000000 +0200
> @@ -21,6 +21,7 @@
> #include <net/ndisc.h>
> #include <net/flow.h>
> #include <net/snmp.h>
> +#include <linux/in.h>
> 
> #define SIN6_LEN_RFC2133    24
> 
> @@ -167,6 +168,12 @@ DECLARE_SNMP_STAT(struct udp_mib, udplit
>     if (is_udplite) SNMP_INC_STATS_USER(udplite_stats_in6, 
> field);         \
>     else        SNMP_INC_STATS_USER(udp_stats_in6, field);    } while(0)
> 
> +#define IS_ADDR_MAPPED(a) \
> +    (((uint32_t *) (a))[0] == 0            \
> +    && ((uint32_t *) (a))[1] == 0            \
> +    && (((uint32_t *) (a))[2] == 0            \
> +    || ((uint32_t *) (a))[2] == htonl(0xffff)))

I need to update a patch of mine that added a v4-mapped inline, let me 
send that out.  In the kernel you should use u32 too, is that why you 
needed to include linux/net.h?

> +/* Maps a IPv4 address into a wright IPv6 address */
> +static inline int ipv6_addr_map(const struct in_addr a1, struct 
> in6_addr a2)
> +{
> +    a2.s6_addr32[0] = 0;
> +    a2.s6_addr32[1] = 0;
> +    a2.s6_addr32[2] = htonl(0xffff);
> +    a2.s6_addr32[3] = (uint32_t)a1.s_addr;
> +    return 0;
> +}

This can be void, noone ever checks the return status.  Maybe change the 
name to ipv6_addr_v4map() too?

> @@ -84,7 +85,7 @@ static void svcauth_unix_domain_release(
> struct ip_map {
>     struct cache_head    h;
>     char            m_class[8]; /* e.g. "nfsd" */
> -    struct in_addr        m_addr;
> +    struct in6_addr        m_addr;

Indentation.

> static void ip_map_init(struct cache_head *cnew, struct cache_head *citem)
> {
> @@ -125,7 +133,7 @@ static void ip_map_init(struct cache_hea
>     struct ip_map *item = container_of(citem, struct ip_map, h);
> 
>     strcpy(new->m_class, item->m_class);
> -    new->m_addr.s_addr = item->m_addr.s_addr;
> +    memcpy(&(new->m_addr), &(item->m_addr), sizeof(struct in6_addr));

Use ipv6_addr_copy().

> @@ -151,20 +159,22 @@ static void ip_map_request(struct cache_
> {
>     char text_addr[20];
>     struct ip_map *im = container_of(h, struct ip_map, h);
> -    __be32 addr = im->m_addr.s_addr;
> -
> -    snprintf(text_addr, 20, "%u.%u.%u.%u",
> -         ntohl(addr) >> 24 & 0xff,
> -         ntohl(addr) >> 16 & 0xff,
> -         ntohl(addr) >>  8 & 0xff,
> -         ntohl(addr) >>  0 & 0xff);
> 
> +    if (IS_ADDR_MAPPED(im->m_addr.s6_addr32)) {
> +        snprintf(text_addr, 20, NIPQUAD_FMT,
> +                ntohl(im->m_addr.s6_addr32[3]) >> 24 & 0xff,
> +                ntohl(im->m_addr.s6_addr32[3]) >> 16 & 0xff,
> +                ntohl(im->m_addr.s6_addr32[3]) >>  8 & 0xff,
> +                ntohl(im->m_addr.s6_addr32[3]) >>  0 & 0xff);
> +    } else {
> +        snprintf(text_addr, 20, NIP6_FMT, NIP6(im->m_addr));
> +    }

You'll need more than 20 bytes to print an IPv6 address, I'd make this 
at least 44 to account for some fluff.  Surprised you didn't crash 
during testing.

> static int ip_map_parse(struct cache_detail *cd,
> @@ -175,10 +185,10 @@ static int ip_map_parse(struct cache_det
>      * for scratch: */
>     char *buf = mesg;
>     int len;
> -    int b1,b2,b3,b4;
> +    int b1, b2, b3, b4, b5, b6, b7, b8;
>     char c;
>     char class[8];
> -    struct in_addr addr;
> +    struct in6_addr addr;
>     int err;

You might as well fix the indentation while you're here.

> @@ -238,7 +262,7 @@ static int ip_map_show(struct seq_file *
>                struct cache_head *h)
> {
>     struct ip_map *im;
> -    struct in_addr addr;
> +    struct in6_addr addr;

Indentation.

>     if (h == NULL) {
> @@ -247,20 +271,33 @@ static int ip_map_show(struct seq_file *
>     }
>     im = container_of(h, struct ip_map, h);
>     /* class addr domain */
> -    addr = im->m_addr;
> +    memcpy(&addr, &im->m_addr, sizeof(struct in6_addr));

ipv6_addr_copy()

> -    seq_printf(m, "%s %d.%d.%d.%d %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,
> -           dom
> -           );
> +    if (IS_ADDR_MAPPED(addr.s6_addr32)) {
> +        seq_printf(m, "%s" NIPQUAD_FMT "%s\n",
> +            im->m_class,
> +            ntohl(addr.s6_addr32[3]) >> 24 & 0xff,
> +            ntohl(addr.s6_addr32[3]) >> 16 & 0xff,
> +            ntohl(addr.s6_addr32[3]) >>  8 & 0xff,
> +            ntohl(addr.s6_addr32[3]) >>  0 & 0xff,
> +            dom);
> +    } else {
> +        seq_printf(m, "%s" NIP6_FMT "%s\n",
> +            im->m_class,
> +            ntohl(addr.s6_addr16[7]),
> +            ntohl(addr.s6_addr16[6]),
> +            ntohl(addr.s6_addr16[5]),
> +            ntohl(addr.s6_addr16[4]),
> +            ntohl(addr.s6_addr16[3]),
> +            ntohl(addr.s6_addr16[2]),
> +            ntohl(addr.s6_addr16[1]),
> +            ntohl(addr.s6_addr16[0]),
> +            dom);

These should be ntohs(), it's only a 16-bit quantity.  Also, don't you 
want to start printing this at addr[0], not addr[7]?

> -static struct ip_map *ip_map_lookup(char *class, struct in_addr addr)
> +static struct ip_map *ip_map_lookup(char *class, struct in6_addr addr)
> {
>     struct ip_map ip;
>     struct cache_head *ch;
> 
>     strcpy(ip.m_class, class);
> -    ip.m_addr = addr;
> +    memcpy(&ip.m_addr, &addr, sizeof(struct in6_addr));

ipv6_addr_copy()

-Brian