* [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
@ 2011-08-20 10:24 Mi Jinlong
2011-08-22 19:23 ` J. Bruce Fields
2011-08-22 19:26 ` J. Bruce Fields
0 siblings, 2 replies; 24+ messages in thread
From: Mi Jinlong @ 2011-08-20 10:24 UTC (permalink / raw)
To: NFS; +Cc: J. Bruce Fields, Trond Myklebust, Chuck Lever
For IPv6 local address, lockd can not callback to client for
missing scope id when binding address at inet6_bind:
324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
325 if (addr_len >= sizeof(struct sockaddr_in6) &&
326 addr->sin6_scope_id) {
327 /* Override any existing binding, if another one
328 * is supplied by user.
329 */
330 sk->sk_bound_dev_if = addr->sin6_scope_id;
331 }
332
333 /* Binding to link-local address requires an interface */
334 if (!sk->sk_bound_dev_if) {
335 err = -EINVAL;
336 goto out_unlock;
337 }
This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
xprt->xpt_local to rqstp->rq_daddr for use.
With this patch, lockd can callback to client success.
Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
fs/lockd/host.c | 3 ++-
fs/nfsd/nfs4state.c | 7 ++++++-
include/linux/sunrpc/svc.h | 12 ++++++++++--
net/sunrpc/svc_xprt.c | 5 ++++-
net/sunrpc/svcsock.c | 4 ++--
5 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index b7c99bf..1fff87a 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -346,7 +346,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
src_sap = (struct sockaddr *)&sin;
break;
case AF_INET6:
- ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
+ ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6.sin6_addr);
+ sin6.sin6_scope_id = rqstp->rq_daddr.addr6.sin6_scope_id;
src_sap = (struct sockaddr *)&sin6;
break;
default:
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3787ec1..1169411 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1182,7 +1182,7 @@ static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, uni
return;
case AF_INET6:
((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
- ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
+ ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6.sin6_addr;
return;
}
}
@@ -1219,6 +1219,11 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
conn->cb_prog = se->se_callback_prog;
conn->cb_ident = se->se_callback_ident;
rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
+
+ if (conn->cb_saddr.ss_family == AF_INET6)
+ ((struct sockaddr_in6 *)&conn->cb_saddr)->sin6_scope_id =
+ rqstp->rq_daddr.addr6.sin6_scope_id;
+
return;
out_err:
conn->cb_addr.ss_family = AF_UNSPEC;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 223588a..cd611b5 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -212,9 +212,17 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
iov->iov_len += sizeof(__be32);
}
+/*
+ * Add scope id for LINKLOCAL address
+ */
+struct in6_addr_scopeid{
+ struct in6_addr sin6_addr;
+ __u32 sin6_scope_id;
+};
+
union svc_addr_u {
- struct in_addr addr;
- struct in6_addr addr6;
+ struct in_addr addr;
+ struct in6_addr_scopeid addr6;
};
/*
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index bd31208..8aada49 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -269,7 +269,10 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
break;
case AF_INET6:
- rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
+ rqstp->rq_daddr.addr6.sin6_addr =
+ ((struct sockaddr_in6 *)sin)->sin6_addr;
+ rqstp->rq_daddr.addr6.sin6_scope_id =
+ ((struct sockaddr_in6 *)sin)->sin6_scope_id;
break;
}
}
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 767d494..beb2575 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -155,7 +155,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
cmh->cmsg_type = IPV6_PKTINFO;
pki->ipi6_ifindex = 0;
ipv6_addr_copy(&pki->ipi6_addr,
- &rqstp->rq_daddr.addr6);
+ &rqstp->rq_daddr.addr6.sin6_addr);
cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
}
break;
@@ -513,7 +513,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
struct in6_pktinfo *pki = CMSG_DATA(cmh);
if (cmh->cmsg_type != IPV6_PKTINFO)
return 0;
- ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
+ ipv6_addr_copy(&rqstp->rq_daddr.addr6.sin6_addr, &pki->ipi6_addr);
return 1;
}
--
1.7.6
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-20 10:24 [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address Mi Jinlong
@ 2011-08-22 19:23 ` J. Bruce Fields
2011-08-22 19:39 ` Jeff Layton
2011-08-22 19:26 ` J. Bruce Fields
1 sibling, 1 reply; 24+ messages in thread
From: J. Bruce Fields @ 2011-08-22 19:23 UTC (permalink / raw)
To: Mi Jinlong; +Cc: NFS, Trond Myklebust, Chuck Lever, jlayton
On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
> xprt->xpt_local to rqstp->rq_daddr for use.
>
> With this patch, lockd can callback to client success.
I guess this makes sense to me, but someone who understands IPv6 better
should comment... Chuck? Jeff?
--b.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 3 ++-
> fs/nfsd/nfs4state.c | 7 ++++++-
> include/linux/sunrpc/svc.h | 12 ++++++++++--
> net/sunrpc/svc_xprt.c | 5 ++++-
> net/sunrpc/svcsock.c | 4 ++--
> 5 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..1fff87a 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -346,7 +346,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> src_sap = (struct sockaddr *)&sin;
> break;
> case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> + ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6.sin6_addr);
> + sin6.sin6_scope_id = rqstp->rq_daddr.addr6.sin6_scope_id;
> src_sap = (struct sockaddr *)&sin6;
> break;
> default:
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..1169411 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1182,7 +1182,7 @@ static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, uni
> return;
> case AF_INET6:
> ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> + ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6.sin6_addr;
> return;
> }
> }
> @@ -1219,6 +1219,11 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> +
> + if (conn->cb_saddr.ss_family == AF_INET6)
> + ((struct sockaddr_in6 *)&conn->cb_saddr)->sin6_scope_id =
> + rqstp->rq_daddr.addr6.sin6_scope_id;
> +
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..cd611b5 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,9 +212,17 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> +/*
> + * Add scope id for LINKLOCAL address
> + */
> +struct in6_addr_scopeid{
> + struct in6_addr sin6_addr;
> + __u32 sin6_scope_id;
> +};
> +
> union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> + struct in_addr addr;
> + struct in6_addr_scopeid addr6;
> };
>
> /*
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..8aada49 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -269,7 +269,10 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> break;
> case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> + rqstp->rq_daddr.addr6.sin6_addr =
> + ((struct sockaddr_in6 *)sin)->sin6_addr;
> + rqstp->rq_daddr.addr6.sin6_scope_id =
> + ((struct sockaddr_in6 *)sin)->sin6_scope_id;
> break;
> }
> }
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..beb2575 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -155,7 +155,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_type = IPV6_PKTINFO;
> pki->ipi6_ifindex = 0;
> ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + &rqstp->rq_daddr.addr6.sin6_addr);
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -513,7 +513,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> + ipv6_addr_copy(&rqstp->rq_daddr.addr6.sin6_addr, &pki->ipi6_addr);
> return 1;
> }
>
> --
> 1.7.6
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-20 10:24 [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address Mi Jinlong
2011-08-22 19:23 ` J. Bruce Fields
@ 2011-08-22 19:26 ` J. Bruce Fields
2011-08-22 19:32 ` Chuck Lever
1 sibling, 1 reply; 24+ messages in thread
From: J. Bruce Fields @ 2011-08-22 19:26 UTC (permalink / raw)
To: Mi Jinlong; +Cc: NFS, Trond Myklebust, Chuck Lever
On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
> +/*
> + * Add scope id for LINKLOCAL address
> + */
> +struct in6_addr_scopeid{
> + struct in6_addr sin6_addr;
> + __u32 sin6_scope_id;
> +};
> +
> union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> + struct in_addr addr;
> + struct in6_addr_scopeid addr6;
By the way, is there any reason why nfsd really needs its own address
structure? Shouldn't we use sockaddr_storage or something? I feel like
we've got a little too much one-off address handling in nfsd.
--b.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:26 ` J. Bruce Fields
@ 2011-08-22 19:32 ` Chuck Lever
2011-08-22 19:44 ` J. Bruce Fields
0 siblings, 1 reply; 24+ messages in thread
From: Chuck Lever @ 2011-08-22 19:32 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Mi Jinlong, NFS, Trond Myklebust
On Aug 22, 2011, at 3:26 PM, J. Bruce Fields wrote:
> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
>> +/*
>> + * Add scope id for LINKLOCAL address
>> + */
>> +struct in6_addr_scopeid{
>> + struct in6_addr sin6_addr;
>> + __u32 sin6_scope_id;
>> +};
>> +
>> union svc_addr_u {
>> - struct in_addr addr;
>> - struct in6_addr addr6;
>> + struct in_addr addr;
>> + struct in6_addr_scopeid addr6;
>
> By the way, is there any reason why nfsd really needs its own address
> structure? Shouldn't we use sockaddr_storage or something? I feel like
> we've got a little too much one-off address handling in nfsd.
That would be my only complaint about the patch.
I think we chose a smaller struct here to save space, and we could do that because we didn't need a port number or scope ID. If a scope ID is indeed required, then we should consider something larger like a struct sockaddr_storage, IMO.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:23 ` J. Bruce Fields
@ 2011-08-22 19:39 ` Jeff Layton
2011-08-22 19:44 ` Chuck Lever
0 siblings, 1 reply; 24+ messages in thread
From: Jeff Layton @ 2011-08-22 19:39 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Mi Jinlong, NFS, Trond Myklebust, Chuck Lever
On Mon, 22 Aug 2011 15:23:48 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
> > For IPv6 local address, lockd can not callback to client for
> > missing scope id when binding address at inet6_bind:
> >
> > 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> > 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> > 326 addr->sin6_scope_id) {
> > 327 /* Override any existing binding, if another one
> > 328 * is supplied by user.
> > 329 */
> > 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> > 331 }
> > 332
> > 333 /* Binding to link-local address requires an interface */
> > 334 if (!sk->sk_bound_dev_if) {
> > 335 err = -EINVAL;
> > 336 goto out_unlock;
> > 337 }
> >
> > This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
> > xprt->xpt_local to rqstp->rq_daddr for use.
> >
> > With this patch, lockd can callback to client success.
>
> I guess this makes sense to me, but someone who understands IPv6 better
> should comment... Chuck? Jeff?
>
Sounds like a reasonable explanation. Link-local addresses all have the
same prefix, so we need a scopeid (aka interface ID# for linux) to know
which interface we should send a call on.
When I did the patches to allow NFSv4 callbacks to go over IPv6, I
did something similar, but used the rq_addr. From gen_callback:
struct sockaddr *sa = svc_addr(rqstp);
u32 scopeid = rpc_get_scope_id(sa);
...and the scopeid was used to populate the scopeid of the callback
address. The rq_daddr should be equivalent though. The _addr and _daddr
must have the same scopeid or something is very wrong...
That said, on a semi-related note...
I have to wonder about statd in conjunction with link-local addresses.
I have a hard time understanding how you'd ever get reboot
notifications in such a setup.
You might be able to make it work if /etc/hosts allowed you to put in a
scopeid for an address, but even then it sounds sketchy...
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:39 ` Jeff Layton
@ 2011-08-22 19:44 ` Chuck Lever
2011-08-22 21:08 ` J. Bruce Fields
2011-08-23 14:43 ` Jeff Layton
0 siblings, 2 replies; 24+ messages in thread
From: Chuck Lever @ 2011-08-22 19:44 UTC (permalink / raw)
To: Jeff Layton; +Cc: J. Bruce Fields, Mi Jinlong, NFS, Trond Myklebust
On Aug 22, 2011, at 3:39 PM, Jeff Layton wrote:
> On Mon, 22 Aug 2011 15:23:48 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>
>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
>>> For IPv6 local address, lockd can not callback to client for
>>> missing scope id when binding address at inet6_bind:
>>>
>>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>>> 326 addr->sin6_scope_id) {
>>> 327 /* Override any existing binding, if another one
>>> 328 * is supplied by user.
>>> 329 */
>>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>>> 331 }
>>> 332
>>> 333 /* Binding to link-local address requires an interface */
>>> 334 if (!sk->sk_bound_dev_if) {
>>> 335 err = -EINVAL;
>>> 336 goto out_unlock;
>>> 337 }
>>>
>>> This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
>>> xprt->xpt_local to rqstp->rq_daddr for use.
>>>
>>> With this patch, lockd can callback to client success.
>>
>> I guess this makes sense to me, but someone who understands IPv6 better
>> should comment... Chuck? Jeff?
>>
>
> Sounds like a reasonable explanation. Link-local addresses all have the
> same prefix, so we need a scopeid (aka interface ID# for linux) to know
> which interface we should send a call on.
>
> When I did the patches to allow NFSv4 callbacks to go over IPv6, I
> did something similar, but used the rq_addr. From gen_callback:
>
> struct sockaddr *sa = svc_addr(rqstp);
> u32 scopeid = rpc_get_scope_id(sa);
>
> ...and the scopeid was used to populate the scopeid of the callback
> address. The rq_daddr should be equivalent though. The _addr and _daddr
> must have the same scopeid or something is very wrong...
Sure, that would preclude the need to upgrade the field to a struct sockaddr_storage.
> That said, on a semi-related note...
>
> I have to wonder about statd in conjunction with link-local addresses.
> I have a hard time understanding how you'd ever get reboot
> notifications in such a setup.
The only way that could work is if DNS was able to look at a local hostname, and provide the correct scope ID. Doubtful. I wonder if mDNS might provide such ability.
> You might be able to make it work if /etc/hosts allowed you to put in a
> scopeid for an address, but even then it sounds sketchy...
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:32 ` Chuck Lever
@ 2011-08-22 19:44 ` J. Bruce Fields
2011-08-23 9:40 ` Mi Jinlong
2011-08-24 5:58 ` Mi Jinlong
0 siblings, 2 replies; 24+ messages in thread
From: J. Bruce Fields @ 2011-08-22 19:44 UTC (permalink / raw)
To: Chuck Lever; +Cc: Mi Jinlong, NFS, Trond Myklebust
On Mon, Aug 22, 2011 at 03:32:13PM -0400, Chuck Lever wrote:
>
> On Aug 22, 2011, at 3:26 PM, J. Bruce Fields wrote:
>
> > On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
> >> +/*
> >> + * Add scope id for LINKLOCAL address
> >> + */
> >> +struct in6_addr_scopeid{
> >> + struct in6_addr sin6_addr;
> >> + __u32 sin6_scope_id;
> >> +};
> >> +
> >> union svc_addr_u {
> >> - struct in_addr addr;
> >> - struct in6_addr addr6;
> >> + struct in_addr addr;
> >> + struct in6_addr_scopeid addr6;
> >
> > By the way, is there any reason why nfsd really needs its own address
> > structure? Shouldn't we use sockaddr_storage or something? I feel like
> > we've got a little too much one-off address handling in nfsd.
>
> That would be my only complaint about the patch.
>
> I think we chose a smaller struct here to save space, and we could do that because we didn't need a port number or scope ID. If a scope ID is indeed required, then we should consider something larger like a struct sockaddr_storage, IMO.
If we care about the size of struct svc_rqst: rq_vec and rq_pages fields
are 4k and 2k, respectively, and the rest is lost in the noise.
Mi Jinlong, would you have the time to try this?
- Replace svc_addr_u by sockaddr_storage
- See of that allows us to delete some lines of code in nfsd by
using standard helper functions instead of copying things by
hand.
You'd do that in one patch, then apply your scope-id fix on top of it as
a second patch.
--b.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:44 ` Chuck Lever
@ 2011-08-22 21:08 ` J. Bruce Fields
2011-08-23 14:43 ` Jeff Layton
1 sibling, 0 replies; 24+ messages in thread
From: J. Bruce Fields @ 2011-08-22 21:08 UTC (permalink / raw)
To: Chuck Lever; +Cc: Jeff Layton, Mi Jinlong, NFS, Trond Myklebust
On Mon, Aug 22, 2011 at 03:44:39PM -0400, Chuck Lever wrote:
> On Aug 22, 2011, at 3:39 PM, Jeff Layton wrote:
> > That said, on a semi-related note...
> >
> > I have to wonder about statd in conjunction with link-local addresses.
> > I have a hard time understanding how you'd ever get reboot
> > notifications in such a setup.
>
> The only way that could work is if DNS was able to look at a local hostname, and provide the correct scope ID. Doubtful. I wonder if mDNS might provide such ability.
OK. Mi Jinlong, if you don't have a use case in mind for this, let's
not bother.
--b.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:44 ` J. Bruce Fields
@ 2011-08-23 9:40 ` Mi Jinlong
2011-08-24 5:58 ` Mi Jinlong
1 sibling, 0 replies; 24+ messages in thread
From: Mi Jinlong @ 2011-08-23 9:40 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Chuck Lever, NFS, Trond Myklebust
J. Bruce Fields:
> On Mon, Aug 22, 2011 at 03:32:13PM -0400, Chuck Lever wrote:
>> On Aug 22, 2011, at 3:26 PM, J. Bruce Fields wrote:
>>
>>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
>>>> +/*
>>>> + * Add scope id for LINKLOCAL address
>>>> + */
>>>> +struct in6_addr_scopeid{
>>>> + struct in6_addr sin6_addr;
>>>> + __u32 sin6_scope_id;
>>>> +};
>>>> +
>>>> union svc_addr_u {
>>>> - struct in_addr addr;
>>>> - struct in6_addr addr6;
>>>> + struct in_addr addr;
>>>> + struct in6_addr_scopeid addr6;
>>> By the way, is there any reason why nfsd really needs its own address
>>> structure? Shouldn't we use sockaddr_storage or something? I feel like
>>> we've got a little too much one-off address handling in nfsd.
>> That would be my only complaint about the patch.
>>
>> I think we chose a smaller struct here to save space, and we could do that because we didn't need a port number or scope ID. If a scope ID is indeed required, then we should consider something larger like a struct sockaddr_storage, IMO.
>
> If we care about the size of struct svc_rqst: rq_vec and rq_pages fields
> are 4k and 2k, respectively, and the rest is lost in the noise.
>
> Mi Jinlong, would you have the time to try this?
OK, I will do this.
Thanks for all comment.
--
----
thanks
Mi Jinlong
>
> - Replace svc_addr_u by sockaddr_storage
> - See of that allows us to delete some lines of code in nfsd by
> using standard helper functions instead of copying things by
> hand.
>
> You'd do that in one patch, then apply your scope-id fix on top of it as
> a second patch.
>
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:44 ` Chuck Lever
2011-08-22 21:08 ` J. Bruce Fields
@ 2011-08-23 14:43 ` Jeff Layton
2011-08-24 8:00 ` Mi Jinlong
1 sibling, 1 reply; 24+ messages in thread
From: Jeff Layton @ 2011-08-23 14:43 UTC (permalink / raw)
To: Chuck Lever; +Cc: J. Bruce Fields, Mi Jinlong, NFS, Trond Myklebust
On Mon, 22 Aug 2011 15:44:39 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On Aug 22, 2011, at 3:39 PM, Jeff Layton wrote:
>
> > On Mon, 22 Aug 2011 15:23:48 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >
> >> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
> >>> For IPv6 local address, lockd can not callback to client for
> >>> missing scope id when binding address at inet6_bind:
> >>>
> >>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> >>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> >>> 326 addr->sin6_scope_id) {
> >>> 327 /* Override any existing binding, if another one
> >>> 328 * is supplied by user.
> >>> 329 */
> >>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> >>> 331 }
> >>> 332
> >>> 333 /* Binding to link-local address requires an interface */
> >>> 334 if (!sk->sk_bound_dev_if) {
> >>> 335 err = -EINVAL;
> >>> 336 goto out_unlock;
> >>> 337 }
> >>>
> >>> This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
> >>> xprt->xpt_local to rqstp->rq_daddr for use.
> >>>
> >>> With this patch, lockd can callback to client success.
> >>
> >> I guess this makes sense to me, but someone who understands IPv6 better
> >> should comment... Chuck? Jeff?
> >>
> >
> > Sounds like a reasonable explanation. Link-local addresses all have the
> > same prefix, so we need a scopeid (aka interface ID# for linux) to know
> > which interface we should send a call on.
> >
> > When I did the patches to allow NFSv4 callbacks to go over IPv6, I
> > did something similar, but used the rq_addr. From gen_callback:
> >
> > struct sockaddr *sa = svc_addr(rqstp);
> > u32 scopeid = rpc_get_scope_id(sa);
> >
> > ...and the scopeid was used to populate the scopeid of the callback
> > address. The rq_daddr should be equivalent though. The _addr and _daddr
> > must have the same scopeid or something is very wrong...
>
> Sure, that would preclude the need to upgrade the field to a struct sockaddr_storage.
>
> > That said, on a semi-related note...
> >
> > I have to wonder about statd in conjunction with link-local addresses.
> > I have a hard time understanding how you'd ever get reboot
> > notifications in such a setup.
>
> The only way that could work is if DNS was able to look at a local hostname, and provide the correct scope ID. Doubtful. I wonder if mDNS might provide such ability.
>
I haven't looked closely at the mDNS protocol, but regular DNS does not
support scopeids on AAAA records.
It does seem somewhat plausible that this could work "accidentally".
The reboot notifications could end up going via routable addresses
instead of the link-local ones if you set nsm_use_hostnames.
NSM is a shaky protocol at best though, so I'd make sure to test this
before relying on it. Mi, if you get this working (including reboot
notifications), please let us know how you solved this...
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-22 19:44 ` J. Bruce Fields
2011-08-23 9:40 ` Mi Jinlong
@ 2011-08-24 5:58 ` Mi Jinlong
2011-08-24 18:39 ` J. Bruce Fields
` (2 more replies)
1 sibling, 3 replies; 24+ messages in thread
From: Mi Jinlong @ 2011-08-24 5:58 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Chuck Lever, NFS, Trond Myklebust
J. Bruce Fields:
> On Mon, Aug 22, 2011 at 03:32:13PM -0400, Chuck Lever wrote:
>> On Aug 22, 2011, at 3:26 PM, J. Bruce Fields wrote:
>>
>>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
>>>> +/*
>>>> + * Add scope id for LINKLOCAL address
>>>> + */
>>>> +struct in6_addr_scopeid{
>>>> + struct in6_addr sin6_addr;
>>>> + __u32 sin6_scope_id;
>>>> +};
>>>> +
>>>> union svc_addr_u {
>>>> - struct in_addr addr;
>>>> - struct in6_addr addr6;
>>>> + struct in_addr addr;
>>>> + struct in6_addr_scopeid addr6;
>>> By the way, is there any reason why nfsd really needs its own address
>>> structure? Shouldn't we use sockaddr_storage or something? I feel like
>>> we've got a little too much one-off address handling in nfsd.
>> That would be my only complaint about the patch.
>>
>> I think we chose a smaller struct here to save space, and we could do that because we didn't need a port number or scope ID. If a scope ID is indeed required, then we should consider something larger like a struct sockaddr_storage, IMO.
>
> If we care about the size of struct svc_rqst: rq_vec and rq_pages fields
> are 4k and 2k, respectively, and the rest is lost in the noise.
>
> Mi Jinlong, would you have the time to try this?
>
> - Replace svc_addr_u by sockaddr_storage
> - See of that allows us to delete some lines of code in nfsd by
> using standard helper functions instead of copying things by
> hand.
>
> You'd do that in one patch, then apply your scope-id fix on top of it as
> a second patch.
After replace svc_addr_u by sockaddr_storage, the scope-id fix isn't needed.
thanks,
Mi Jinlong
>From 86bb57f2d7c29124ae73bd121bb6b97a72fe6f4d Mon Sep 17 00:00:00 2001
From: Mi Jinlong <mijinlong@cn.fujitsu.com>
Date: Mon, 24 Aug 2011 13:22:39 +0800
Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
For IPv6 local address, lockd can not callback to client for
missing scope id when binding address at inet6_bind:
324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
325 if (addr_len >= sizeof(struct sockaddr_in6) &&
326 addr->sin6_scope_id) {
327 /* Override any existing binding, if another one
328 * is supplied by user.
329 */
330 sk->sk_bound_dev_if = addr->sin6_scope_id;
331 }
332
333 /* Binding to link-local address requires an interface */
334 if (!sk->sk_bound_dev_if) {
335 err = -EINVAL;
336 goto out_unlock;
337 }
Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
besides address.
Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
fs/lockd/host.c | 14 ++------------
fs/nfsd/nfs4state.c | 16 +---------------
include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
net/sunrpc/svc_xprt.c | 13 ++-----------
net/sunrpc/svcsock.c | 9 +++++----
5 files changed, 31 insertions(+), 51 deletions(-)
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index b7c99bf..b23db05 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
struct hlist_node *pos;
struct nlm_host *host = NULL;
struct nsm_handle *nsm = NULL;
- struct sockaddr_in sin = {
- .sin_family = AF_INET,
- };
- struct sockaddr_in6 sin6 = {
- .sin6_family = AF_INET6,
- };
struct sockaddr *src_sap;
- size_t src_len = rqstp->rq_addrlen;
+ size_t src_len = rqstp->rq_daddrlen;
struct nlm_lookup_host_info ni = {
.server = 1,
.sap = svc_addr(rqstp),
@@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
switch (ni.sap->sa_family) {
case AF_INET:
- sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
- src_sap = (struct sockaddr *)&sin;
- break;
case AF_INET6:
- ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
- src_sap = (struct sockaddr *)&sin6;
+ src_sap = svc_daddr(rqstp);
break;
default:
dprintk("lockd: %s failed; unrecognized address family\n",
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3787ec1..07a18a5 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
return NULL;
}
-static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
-{
- switch (family) {
- case AF_INET:
- ((struct sockaddr_in *)sa)->sin_family = AF_INET;
- ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
- return;
- case AF_INET6:
- ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
- ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
- return;
- }
-}
-
static void
gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
{
@@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
conn->cb_prog = se->se_callback_prog;
conn->cb_ident = se->se_callback_ident;
- rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
+ memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
return;
out_err:
conn->cb_addr.ss_family = AF_UNSPEC;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 223588a..3bc0dc9 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
iov->iov_len += sizeof(__be32);
}
-union svc_addr_u {
- struct in_addr addr;
- struct in6_addr addr6;
-};
-
/*
* The context of a single thread, including the request currently being
* processed.
@@ -225,8 +220,12 @@ struct svc_rqst {
struct list_head rq_list; /* idle list */
struct list_head rq_all; /* all threads list */
struct svc_xprt * rq_xprt; /* transport ptr */
+
struct sockaddr_storage rq_addr; /* peer address */
size_t rq_addrlen;
+ struct sockaddr_storage rq_daddr; /* dest addr of request
+ * - reply from here */
+ size_t rq_daddrlen;
struct svc_serv * rq_server; /* RPC service definition */
struct svc_pool * rq_pool; /* thread pool */
@@ -255,9 +254,6 @@ struct svc_rqst {
unsigned short
rq_secure : 1; /* secure port */
- union svc_addr_u rq_daddr; /* dest addr of request
- * - reply from here */
-
void * rq_argp; /* decoded arguments */
void * rq_resp; /* xdr'd results */
void * rq_auth_data; /* flavor-specific data */
@@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
return (struct sockaddr *) &rqst->rq_addr;
}
+static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
+{
+ return (struct sockaddr_in *) &rqst->rq_daddr;
+}
+
+static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
+{
+ return (struct sockaddr_in6 *) &rqst->rq_daddr;
+}
+
+static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
+{
+ return (struct sockaddr *) &rqst->rq_daddr;
+}
+
/*
* Check buffer bounds after decoding arguments
*/
@@ -340,7 +351,8 @@ struct svc_deferred_req {
struct svc_xprt *xprt;
struct sockaddr_storage addr; /* where reply must go */
size_t addrlen;
- union svc_addr_u daddr; /* where reply must come from */
+ struct sockaddr_storage daddr; /* where reply must come from */
+ size_t daddrlen;
struct cache_deferred_req handle;
size_t xprt_hlen;
int argslen;
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index bd31208..d86bb67 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
*/
void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
{
- struct sockaddr *sin;
-
memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
rqstp->rq_addrlen = xprt->xpt_remotelen;
@@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
* Destination address in request is needed for binding the
* source address in RPC replies/callbacks later.
*/
- sin = (struct sockaddr *)&xprt->xpt_local;
- switch (sin->sa_family) {
- case AF_INET:
- rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
- break;
- case AF_INET6:
- rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
- break;
- }
+ memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
+ rqstp->rq_daddrlen = xprt->xpt_locallen;
}
EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 767d494..b9f4ad4 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
cmh->cmsg_level = SOL_IP;
cmh->cmsg_type = IP_PKTINFO;
pki->ipi_ifindex = 0;
- pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
+ pki->ipi_spec_dst.s_addr =
+ svc_daddr_in(rqstp)->sin_addr.s_addr;
cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
}
break;
@@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
cmh->cmsg_type = IPV6_PKTINFO;
pki->ipi6_ifindex = 0;
ipv6_addr_copy(&pki->ipi6_addr,
- &rqstp->rq_daddr.addr6);
+ &svc_daddr_in6(rqstp)->sin6_addr);
cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
}
break;
@@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
struct in_pktinfo *pki = CMSG_DATA(cmh);
if (cmh->cmsg_type != IP_PKTINFO)
return 0;
- rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
+ svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
return 1;
}
@@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
struct in6_pktinfo *pki = CMSG_DATA(cmh);
if (cmh->cmsg_type != IPV6_PKTINFO)
return 0;
- ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
+ ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
return 1;
}
--
1.7.6
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-23 14:43 ` Jeff Layton
@ 2011-08-24 8:00 ` Mi Jinlong
2011-08-24 15:15 ` Chuck Lever
0 siblings, 1 reply; 24+ messages in thread
From: Mi Jinlong @ 2011-08-24 8:00 UTC (permalink / raw)
To: Jeff Layton; +Cc: Chuck Lever, J. Bruce Fields, NFS, Trond Myklebust
Jeff Layton :
> On Mon, 22 Aug 2011 15:44:39 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
>> On Aug 22, 2011, at 3:39 PM, Jeff Layton wrote:
>>
>>> On Mon, 22 Aug 2011 15:23:48 -0400
>>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>>
>>>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
>>>>> For IPv6 local address, lockd can not callback to client for
>>>>> missing scope id when binding address at inet6_bind:
>>>>>
>>>>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>>>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>>>>> 326 addr->sin6_scope_id) {
>>>>> 327 /* Override any existing binding, if another one
>>>>> 328 * is supplied by user.
>>>>> 329 */
>>>>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>>>>> 331 }
>>>>> 332
>>>>> 333 /* Binding to link-local address requires an interface */
>>>>> 334 if (!sk->sk_bound_dev_if) {
>>>>> 335 err = -EINVAL;
>>>>> 336 goto out_unlock;
>>>>> 337 }
>>>>>
>>>>> This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
>>>>> xprt->xpt_local to rqstp->rq_daddr for use.
>>>>>
>>>>> With this patch, lockd can callback to client success.
>>>> I guess this makes sense to me, but someone who understands IPv6 better
>>>> should comment... Chuck? Jeff?
>>>>
>>> Sounds like a reasonable explanation. Link-local addresses all have the
>>> same prefix, so we need a scopeid (aka interface ID# for linux) to know
>>> which interface we should send a call on.
>>>
>>> When I did the patches to allow NFSv4 callbacks to go over IPv6, I
>>> did something similar, but used the rq_addr. From gen_callback:
>>>
>>> struct sockaddr *sa = svc_addr(rqstp);
>>> u32 scopeid = rpc_get_scope_id(sa);
>>>
>>> ...and the scopeid was used to populate the scopeid of the callback
>>> address. The rq_daddr should be equivalent though. The _addr and _daddr
>>> must have the same scopeid or something is very wrong...
>> Sure, that would preclude the need to upgrade the field to a struct sockaddr_storage.
>>
>>> That said, on a semi-related note...
>>>
>>> I have to wonder about statd in conjunction with link-local addresses.
>>> I have a hard time understanding how you'd ever get reboot
>>> notifications in such a setup.
>> The only way that could work is if DNS was able to look at a local hostname, and provide the correct scope ID. Doubtful. I wonder if mDNS might provide such ability.
>>
>
> I haven't looked closely at the mDNS protocol, but regular DNS does not
> support scopeids on AAAA records.
>
> It does seem somewhat plausible that this could work "accidentally".
> The reboot notifications could end up going via routable addresses
> instead of the link-local ones if you set nsm_use_hostnames.
>
> NSM is a shaky protocol at best though, so I'd make sure to test this
> before relying on it. Mi, if you get this working (including reboot
> notifications), please let us know how you solved this...
Hi Jeff,
I'm not working on NSM. It seems there are some problem of the
latest statd from Steve's git tee.
If having time, I will check the latest statd and sm-notify.
thanks,
Mi Jinlong
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-24 8:00 ` Mi Jinlong
@ 2011-08-24 15:15 ` Chuck Lever
0 siblings, 0 replies; 24+ messages in thread
From: Chuck Lever @ 2011-08-24 15:15 UTC (permalink / raw)
To: Mi Jinlong; +Cc: Jeff Layton, J. Bruce Fields, NFS, Trond Myklebust
On Aug 24, 2011, at 4:00 AM, Mi Jinlong wrote:
> Jeff Layton :
>> On Mon, 22 Aug 2011 15:44:39 -0400
>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>> On Aug 22, 2011, at 3:39 PM, Jeff Layton wrote:
>>>
>>>> On Mon, 22 Aug 2011 15:23:48 -0400
>>>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>>>
>>>>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
>>>>>> For IPv6 local address, lockd can not callback to client for
>>>>>> missing scope id when binding address at inet6_bind:
>>>>>>
>>>>>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>>>>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>>>>>> 326 addr->sin6_scope_id) {
>>>>>> 327 /* Override any existing binding, if another one
>>>>>> 328 * is supplied by user.
>>>>>> 329 */
>>>>>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>>>>>> 331 }
>>>>>> 332
>>>>>> 333 /* Binding to link-local address requires an interface */
>>>>>> 334 if (!sk->sk_bound_dev_if) {
>>>>>> 335 err = -EINVAL;
>>>>>> 336 goto out_unlock;
>>>>>> 337 }
>>>>>>
>>>>>> This patch adds scope id to svc_addr_u for IPv6 address, and copy scope from
>>>>>> xprt->xpt_local to rqstp->rq_daddr for use.
>>>>>>
>>>>>> With this patch, lockd can callback to client success.
>>>>> I guess this makes sense to me, but someone who understands IPv6 better
>>>>> should comment... Chuck? Jeff?
>>>>>
>>>> Sounds like a reasonable explanation. Link-local addresses all have the
>>>> same prefix, so we need a scopeid (aka interface ID# for linux) to know
>>>> which interface we should send a call on.
>>>>
>>>> When I did the patches to allow NFSv4 callbacks to go over IPv6, I
>>>> did something similar, but used the rq_addr. From gen_callback:
>>>>
>>>> struct sockaddr *sa = svc_addr(rqstp);
>>>> u32 scopeid = rpc_get_scope_id(sa);
>>>>
>>>> ...and the scopeid was used to populate the scopeid of the callback
>>>> address. The rq_daddr should be equivalent though. The _addr and _daddr
>>>> must have the same scopeid or something is very wrong...
>>> Sure, that would preclude the need to upgrade the field to a struct sockaddr_storage.
>>>
>>>> That said, on a semi-related note...
>>>>
>>>> I have to wonder about statd in conjunction with link-local addresses.
>>>> I have a hard time understanding how you'd ever get reboot
>>>> notifications in such a setup.
>>> The only way that could work is if DNS was able to look at a local hostname, and provide the correct scope ID. Doubtful. I wonder if mDNS might provide such ability.
>>>
>>
>> I haven't looked closely at the mDNS protocol, but regular DNS does not
>> support scopeids on AAAA records.
>>
>> It does seem somewhat plausible that this could work "accidentally".
>> The reboot notifications could end up going via routable addresses
>> instead of the link-local ones if you set nsm_use_hostnames.
>>
>> NSM is a shaky protocol at best though, so I'd make sure to test this
>> before relying on it. Mi, if you get this working (including reboot
>> notifications), please let us know how you solved this...
>
> Hi Jeff,
>
> I'm not working on NSM. It seems there are some problem of the
> latest statd from Steve's git tee.
I kind of agree that NSM can't work with link-local IPv6 because of its dependence on DNS, but NFSv4 probably should work.
> If having time, I will check the latest statd and sm-notify.
It's a reasonable regression test for your patch :-)
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-24 5:58 ` Mi Jinlong
@ 2011-08-24 18:39 ` J. Bruce Fields
2011-08-25 0:02 ` Mi Jinlong
2011-08-25 9:58 ` Jeff Layton
2011-08-25 14:30 ` Chuck Lever
2 siblings, 1 reply; 24+ messages in thread
From: J. Bruce Fields @ 2011-08-24 18:39 UTC (permalink / raw)
To: Mi Jinlong; +Cc: Chuck Lever, NFS, Trond Myklebust
On Wed, Aug 24, 2011 at 01:58:21PM +0800, Mi Jinlong wrote:
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> besides address.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 14 ++------------
> fs/nfsd/nfs4state.c | 16 +---------------
> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> net/sunrpc/svc_xprt.c | 13 ++-----------
> net/sunrpc/svcsock.c | 9 +++++----
> 5 files changed, 31 insertions(+), 51 deletions(-)
Can't complain about that diffstat!
How are you testing this?
--b.
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..b23db05 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> struct hlist_node *pos;
> struct nlm_host *host = NULL;
> struct nsm_handle *nsm = NULL;
> - struct sockaddr_in sin = {
> - .sin_family = AF_INET,
> - };
> - struct sockaddr_in6 sin6 = {
> - .sin6_family = AF_INET6,
> - };
> struct sockaddr *src_sap;
> - size_t src_len = rqstp->rq_addrlen;
> + size_t src_len = rqstp->rq_daddrlen;
> struct nlm_lookup_host_info ni = {
> .server = 1,
> .sap = svc_addr(rqstp),
> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>
> switch (ni.sap->sa_family) {
> case AF_INET:
> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> - src_sap = (struct sockaddr *)&sin;
> - break;
> case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> - src_sap = (struct sockaddr *)&sin6;
> + src_sap = svc_daddr(rqstp);
> break;
> default:
> dprintk("lockd: %s failed; unrecognized address family\n",
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..07a18a5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> return NULL;
> }
>
> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
> -{
> - switch (family) {
> - case AF_INET:
> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
> - return;
> - case AF_INET6:
> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> - return;
> - }
> -}
> -
> static void
> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
> {
> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..3bc0dc9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> -union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> -};
> -
> /*
> * The context of a single thread, including the request currently being
> * processed.
> @@ -225,8 +220,12 @@ struct svc_rqst {
> struct list_head rq_list; /* idle list */
> struct list_head rq_all; /* all threads list */
> struct svc_xprt * rq_xprt; /* transport ptr */
> +
> struct sockaddr_storage rq_addr; /* peer address */
> size_t rq_addrlen;
> + struct sockaddr_storage rq_daddr; /* dest addr of request
> + * - reply from here */
> + size_t rq_daddrlen;
>
> struct svc_serv * rq_server; /* RPC service definition */
> struct svc_pool * rq_pool; /* thread pool */
> @@ -255,9 +254,6 @@ struct svc_rqst {
> unsigned short
> rq_secure : 1; /* secure port */
>
> - union svc_addr_u rq_daddr; /* dest addr of request
> - * - reply from here */
> -
> void * rq_argp; /* decoded arguments */
> void * rq_resp; /* xdr'd results */
> void * rq_auth_data; /* flavor-specific data */
> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_addr;
> }
>
> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr *) &rqst->rq_daddr;
> +}
> +
> /*
> * Check buffer bounds after decoding arguments
> */
> @@ -340,7 +351,8 @@ struct svc_deferred_req {
> struct svc_xprt *xprt;
> struct sockaddr_storage addr; /* where reply must go */
> size_t addrlen;
> - union svc_addr_u daddr; /* where reply must come from */
> + struct sockaddr_storage daddr; /* where reply must come from */
> + size_t daddrlen;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..d86bb67 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
> */
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> {
> - struct sockaddr *sin;
> -
> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
> rqstp->rq_addrlen = xprt->xpt_remotelen;
>
> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * Destination address in request is needed for binding the
> * source address in RPC replies/callbacks later.
> */
> - sin = (struct sockaddr *)&xprt->xpt_local;
> - switch (sin->sa_family) {
> - case AF_INET:
> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> - break;
> - case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> - break;
> - }
> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
> + rqstp->rq_daddrlen = xprt->xpt_locallen;
> }
> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..b9f4ad4 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_level = SOL_IP;
> cmh->cmsg_type = IP_PKTINFO;
> pki->ipi_ifindex = 0;
> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
> + pki->ipi_spec_dst.s_addr =
> + svc_daddr_in(rqstp)->sin_addr.s_addr;
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_type = IPV6_PKTINFO;
> pki->ipi6_ifindex = 0;
> ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + &svc_daddr_in6(rqstp)->sin6_addr);
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> struct in_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IP_PKTINFO)
> return 0;
> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> return 1;
> }
>
> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
> return 1;
> }
>
> --
> 1.7.6
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-24 18:39 ` J. Bruce Fields
@ 2011-08-25 0:02 ` Mi Jinlong
0 siblings, 0 replies; 24+ messages in thread
From: Mi Jinlong @ 2011-08-25 0:02 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Chuck Lever, NFS, Trond Myklebust
J. Bruce Fields:
> On Wed, Aug 24, 2011 at 01:58:21PM +0800, Mi Jinlong wrote:
>> For IPv6 local address, lockd can not callback to client for
>> missing scope id when binding address at inet6_bind:
>>
>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>> 326 addr->sin6_scope_id) {
>> 327 /* Override any existing binding, if another one
>> 328 * is supplied by user.
>> 329 */
>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>> 331 }
>> 332
>> 333 /* Binding to link-local address requires an interface */
>> 334 if (!sk->sk_bound_dev_if) {
>> 335 err = -EINVAL;
>> 336 goto out_unlock;
>> 337 }
>>
>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
>> besides address.
>>
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>> fs/lockd/host.c | 14 ++------------
>> fs/nfsd/nfs4state.c | 16 +---------------
>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
>> net/sunrpc/svc_xprt.c | 13 ++-----------
>> net/sunrpc/svcsock.c | 9 +++++----
>> 5 files changed, 31 insertions(+), 51 deletions(-)
>
> Can't complain about that diffstat!
>
> How are you testing this?
Just case of:
0. Start tcpdump at nfs server.
1. Two clients open nfsv3 file.
2. ClientA gets a write lock from server.
3. ClientB tries to get a block write lock from server
will blocked.
4. ClientA releases it's write lock which got at step2.
5. ClientB will back from step3's block.
After all steps, I check packages at tcpdump file.
thanks,
Mi Jinlong
>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index b7c99bf..b23db05 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>> struct hlist_node *pos;
>> struct nlm_host *host = NULL;
>> struct nsm_handle *nsm = NULL;
>> - struct sockaddr_in sin = {
>> - .sin_family = AF_INET,
>> - };
>> - struct sockaddr_in6 sin6 = {
>> - .sin6_family = AF_INET6,
>> - };
>> struct sockaddr *src_sap;
>> - size_t src_len = rqstp->rq_addrlen;
>> + size_t src_len = rqstp->rq_daddrlen;
>> struct nlm_lookup_host_info ni = {
>> .server = 1,
>> .sap = svc_addr(rqstp),
>> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>
>> switch (ni.sap->sa_family) {
>> case AF_INET:
>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
>> - src_sap = (struct sockaddr *)&sin;
>> - break;
>> case AF_INET6:
>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
>> - src_sap = (struct sockaddr *)&sin6;
>> + src_sap = svc_daddr(rqstp);
>> break;
>> default:
>> dprintk("lockd: %s failed; unrecognized address family\n",
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 3787ec1..07a18a5 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>> return NULL;
>> }
>>
>> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
>> -{
>> - switch (family) {
>> - case AF_INET:
>> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
>> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
>> - return;
>> - case AF_INET6:
>> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
>> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
>> - return;
>> - }
>> -}
>> -
>> static void
>> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
>> {
>> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>>
>> conn->cb_prog = se->se_callback_prog;
>> conn->cb_ident = se->se_callback_ident;
>> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
>> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
>> return;
>> out_err:
>> conn->cb_addr.ss_family = AF_UNSPEC;
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 223588a..3bc0dc9 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
>> iov->iov_len += sizeof(__be32);
>> }
>>
>> -union svc_addr_u {
>> - struct in_addr addr;
>> - struct in6_addr addr6;
>> -};
>> -
>> /*
>> * The context of a single thread, including the request currently being
>> * processed.
>> @@ -225,8 +220,12 @@ struct svc_rqst {
>> struct list_head rq_list; /* idle list */
>> struct list_head rq_all; /* all threads list */
>> struct svc_xprt * rq_xprt; /* transport ptr */
>> +
>> struct sockaddr_storage rq_addr; /* peer address */
>> size_t rq_addrlen;
>> + struct sockaddr_storage rq_daddr; /* dest addr of request
>> + * - reply from here */
>> + size_t rq_daddrlen;
>>
>> struct svc_serv * rq_server; /* RPC service definition */
>> struct svc_pool * rq_pool; /* thread pool */
>> @@ -255,9 +254,6 @@ struct svc_rqst {
>> unsigned short
>> rq_secure : 1; /* secure port */
>>
>> - union svc_addr_u rq_daddr; /* dest addr of request
>> - * - reply from here */
>> -
>> void * rq_argp; /* decoded arguments */
>> void * rq_resp; /* xdr'd results */
>> void * rq_auth_data; /* flavor-specific data */
>> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
>> return (struct sockaddr *) &rqst->rq_addr;
>> }
>>
>> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr *) &rqst->rq_daddr;
>> +}
>> +
>> /*
>> * Check buffer bounds after decoding arguments
>> */
>> @@ -340,7 +351,8 @@ struct svc_deferred_req {
>> struct svc_xprt *xprt;
>> struct sockaddr_storage addr; /* where reply must go */
>> size_t addrlen;
>> - union svc_addr_u daddr; /* where reply must come from */
>> + struct sockaddr_storage daddr; /* where reply must come from */
>> + size_t daddrlen;
>> struct cache_deferred_req handle;
>> size_t xprt_hlen;
>> int argslen;
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index bd31208..d86bb67 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
>> */
>> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> {
>> - struct sockaddr *sin;
>> -
>> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
>> rqstp->rq_addrlen = xprt->xpt_remotelen;
>>
>> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> * Destination address in request is needed for binding the
>> * source address in RPC replies/callbacks later.
>> */
>> - sin = (struct sockaddr *)&xprt->xpt_local;
>> - switch (sin->sa_family) {
>> - case AF_INET:
>> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
>> - break;
>> - case AF_INET6:
>> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
>> - break;
>> - }
>> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
>> + rqstp->rq_daddrlen = xprt->xpt_locallen;
>> }
>> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 767d494..b9f4ad4 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>> cmh->cmsg_level = SOL_IP;
>> cmh->cmsg_type = IP_PKTINFO;
>> pki->ipi_ifindex = 0;
>> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
>> + pki->ipi_spec_dst.s_addr =
>> + svc_daddr_in(rqstp)->sin_addr.s_addr;
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>> cmh->cmsg_type = IPV6_PKTINFO;
>> pki->ipi6_ifindex = 0;
>> ipv6_addr_copy(&pki->ipi6_addr,
>> - &rqstp->rq_daddr.addr6);
>> + &svc_daddr_in6(rqstp)->sin6_addr);
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
>> struct in_pktinfo *pki = CMSG_DATA(cmh);
>> if (cmh->cmsg_type != IP_PKTINFO)
>> return 0;
>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>> return 1;
>> }
>>
>> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> if (cmh->cmsg_type != IPV6_PKTINFO)
>> return 0;
>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
>> return 1;
>> }
>>
>> --
>> 1.7.6
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
----
thanks
Mi Jinlong
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-24 5:58 ` Mi Jinlong
2011-08-24 18:39 ` J. Bruce Fields
@ 2011-08-25 9:58 ` Jeff Layton
2011-08-25 14:30 ` Chuck Lever
2 siblings, 0 replies; 24+ messages in thread
From: Jeff Layton @ 2011-08-25 9:58 UTC (permalink / raw)
To: Mi Jinlong; +Cc: J. Bruce Fields, Chuck Lever, NFS, Trond Myklebust
On Wed, 24 Aug 2011 13:58:21 +0800
Mi Jinlong <mijinlong@cn.fujitsu.com> wrote:
>
> J. Bruce Fields:
> > On Mon, Aug 22, 2011 at 03:32:13PM -0400, Chuck Lever wrote:
> >> On Aug 22, 2011, at 3:26 PM, J. Bruce Fields wrote:
> >>
> >>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote:
> >>>> +/*
> >>>> + * Add scope id for LINKLOCAL address
> >>>> + */
> >>>> +struct in6_addr_scopeid{
> >>>> + struct in6_addr sin6_addr;
> >>>> + __u32 sin6_scope_id;
> >>>> +};
> >>>> +
> >>>> union svc_addr_u {
> >>>> - struct in_addr addr;
> >>>> - struct in6_addr addr6;
> >>>> + struct in_addr addr;
> >>>> + struct in6_addr_scopeid addr6;
> >>> By the way, is there any reason why nfsd really needs its own address
> >>> structure? Shouldn't we use sockaddr_storage or something? I feel like
> >>> we've got a little too much one-off address handling in nfsd.
> >> That would be my only complaint about the patch.
> >>
> >> I think we chose a smaller struct here to save space, and we could do that because we didn't need a port number or scope ID. If a scope ID is indeed required, then we should consider something larger like a struct sockaddr_storage, IMO.
> >
> > If we care about the size of struct svc_rqst: rq_vec and rq_pages fields
> > are 4k and 2k, respectively, and the rest is lost in the noise.
> >
> > Mi Jinlong, would you have the time to try this?
> >
> > - Replace svc_addr_u by sockaddr_storage
> > - See of that allows us to delete some lines of code in nfsd by
> > using standard helper functions instead of copying things by
> > hand.
> >
> > You'd do that in one patch, then apply your scope-id fix on top of it as
> > a second patch.
>
> After replace svc_addr_u by sockaddr_storage, the scope-id fix isn't needed.
>
> thanks,
> Mi Jinlong
>
> From 86bb57f2d7c29124ae73bd121bb6b97a72fe6f4d Mon Sep 17 00:00:00 2001
> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> Date: Mon, 24 Aug 2011 13:22:39 +0800
> Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
>
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> besides address.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 14 ++------------
> fs/nfsd/nfs4state.c | 16 +---------------
> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> net/sunrpc/svc_xprt.c | 13 ++-----------
> net/sunrpc/svcsock.c | 9 +++++----
> 5 files changed, 31 insertions(+), 51 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..b23db05 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> struct hlist_node *pos;
> struct nlm_host *host = NULL;
> struct nsm_handle *nsm = NULL;
> - struct sockaddr_in sin = {
> - .sin_family = AF_INET,
> - };
> - struct sockaddr_in6 sin6 = {
> - .sin6_family = AF_INET6,
> - };
> struct sockaddr *src_sap;
> - size_t src_len = rqstp->rq_addrlen;
> + size_t src_len = rqstp->rq_daddrlen;
> struct nlm_lookup_host_info ni = {
> .server = 1,
> .sap = svc_addr(rqstp),
> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>
> switch (ni.sap->sa_family) {
> case AF_INET:
> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> - src_sap = (struct sockaddr *)&sin;
> - break;
> case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> - src_sap = (struct sockaddr *)&sin6;
> + src_sap = svc_daddr(rqstp);
> break;
> default:
> dprintk("lockd: %s failed; unrecognized address family\n",
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..07a18a5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> return NULL;
> }
>
> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
> -{
> - switch (family) {
> - case AF_INET:
> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
> - return;
> - case AF_INET6:
> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> - return;
> - }
> -}
> -
> static void
> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
> {
> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..3bc0dc9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> -union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> -};
> -
> /*
> * The context of a single thread, including the request currently being
> * processed.
> @@ -225,8 +220,12 @@ struct svc_rqst {
> struct list_head rq_list; /* idle list */
> struct list_head rq_all; /* all threads list */
> struct svc_xprt * rq_xprt; /* transport ptr */
> +
> struct sockaddr_storage rq_addr; /* peer address */
> size_t rq_addrlen;
> + struct sockaddr_storage rq_daddr; /* dest addr of request
> + * - reply from here */
> + size_t rq_daddrlen;
>
> struct svc_serv * rq_server; /* RPC service definition */
> struct svc_pool * rq_pool; /* thread pool */
> @@ -255,9 +254,6 @@ struct svc_rqst {
> unsigned short
> rq_secure : 1; /* secure port */
>
> - union svc_addr_u rq_daddr; /* dest addr of request
> - * - reply from here */
> -
> void * rq_argp; /* decoded arguments */
> void * rq_resp; /* xdr'd results */
> void * rq_auth_data; /* flavor-specific data */
> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_addr;
> }
>
> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr *) &rqst->rq_daddr;
> +}
> +
> /*
> * Check buffer bounds after decoding arguments
> */
> @@ -340,7 +351,8 @@ struct svc_deferred_req {
> struct svc_xprt *xprt;
> struct sockaddr_storage addr; /* where reply must go */
> size_t addrlen;
> - union svc_addr_u daddr; /* where reply must come from */
> + struct sockaddr_storage daddr; /* where reply must come from */
> + size_t daddrlen;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..d86bb67 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
> */
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> {
> - struct sockaddr *sin;
> -
> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
> rqstp->rq_addrlen = xprt->xpt_remotelen;
>
> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * Destination address in request is needed for binding the
> * source address in RPC replies/callbacks later.
> */
> - sin = (struct sockaddr *)&xprt->xpt_local;
> - switch (sin->sa_family) {
> - case AF_INET:
> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> - break;
> - case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> - break;
> - }
> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
> + rqstp->rq_daddrlen = xprt->xpt_locallen;
> }
> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..b9f4ad4 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_level = SOL_IP;
> cmh->cmsg_type = IP_PKTINFO;
> pki->ipi_ifindex = 0;
> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
> + pki->ipi_spec_dst.s_addr =
> + svc_daddr_in(rqstp)->sin_addr.s_addr;
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_type = IPV6_PKTINFO;
> pki->ipi6_ifindex = 0;
> ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + &svc_daddr_in6(rqstp)->sin6_addr);
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> struct in_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IP_PKTINFO)
> return 0;
> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> return 1;
> }
>
> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
> return 1;
> }
>
Looks good to me.
Reviewed-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-24 5:58 ` Mi Jinlong
2011-08-24 18:39 ` J. Bruce Fields
2011-08-25 9:58 ` Jeff Layton
@ 2011-08-25 14:30 ` Chuck Lever
2011-08-25 14:37 ` Chuck Lever
2 siblings, 1 reply; 24+ messages in thread
From: Chuck Lever @ 2011-08-25 14:30 UTC (permalink / raw)
To: Mi Jinlong; +Cc: J. Bruce Fields, NFS, Trond Myklebust
This seems like a good direction to go in. More comments below.
On Aug 24, 2011, at 1:58 AM, Mi Jinlong wrote:
> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> Date: Mon, 24 Aug 2011 13:22:39 +0800
> Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
>
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> besides address.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 14 ++------------
> fs/nfsd/nfs4state.c | 16 +---------------
> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> net/sunrpc/svc_xprt.c | 13 ++-----------
> net/sunrpc/svcsock.c | 9 +++++----
> 5 files changed, 31 insertions(+), 51 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..b23db05 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> struct hlist_node *pos;
> struct nlm_host *host = NULL;
> struct nsm_handle *nsm = NULL;
> - struct sockaddr_in sin = {
> - .sin_family = AF_INET,
> - };
> - struct sockaddr_in6 sin6 = {
> - .sin6_family = AF_INET6,
> - };
> struct sockaddr *src_sap;
> - size_t src_len = rqstp->rq_addrlen;
> + size_t src_len = rqstp->rq_daddrlen;
> struct nlm_lookup_host_info ni = {
> .server = 1,
> .sap = svc_addr(rqstp),
> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>
> switch (ni.sap->sa_family) {
> case AF_INET:
> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> - src_sap = (struct sockaddr *)&sin;
> - break;
> case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> - src_sap = (struct sockaddr *)&sin6;
> + src_sap = svc_daddr(rqstp);
> break;
> default:
> dprintk("lockd: %s failed; unrecognized address family\n",
With the destination address promoted to a full sockaddr, we probably do not need this switch any more.
I notice that the IPv6 fork of rpc_cmp_addr() does not consider the scope IDs of the passed-in addresses. Perhaps it should?
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..07a18a5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> return NULL;
> }
>
> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
> -{
> - switch (family) {
> - case AF_INET:
> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
> - return;
> - case AF_INET6:
> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> - return;
> - }
> -}
> -
> static void
> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
> {
> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..3bc0dc9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> -union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> -};
> -
> /*
> * The context of a single thread, including the request currently being
> * processed.
> @@ -225,8 +220,12 @@ struct svc_rqst {
> struct list_head rq_list; /* idle list */
> struct list_head rq_all; /* all threads list */
> struct svc_xprt * rq_xprt; /* transport ptr */
> +
> struct sockaddr_storage rq_addr; /* peer address */
> size_t rq_addrlen;
> + struct sockaddr_storage rq_daddr; /* dest addr of request
> + * - reply from here */
> + size_t rq_daddrlen;
>
> struct svc_serv * rq_server; /* RPC service definition */
> struct svc_pool * rq_pool; /* thread pool */
> @@ -255,9 +254,6 @@ struct svc_rqst {
> unsigned short
> rq_secure : 1; /* secure port */
>
> - union svc_addr_u rq_daddr; /* dest addr of request
> - * - reply from here */
> -
> void * rq_argp; /* decoded arguments */
> void * rq_resp; /* xdr'd results */
> void * rq_auth_data; /* flavor-specific data */
> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_addr;
> }
>
> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr *) &rqst->rq_daddr;
> +}
> +
> /*
> * Check buffer bounds after decoding arguments
> */
> @@ -340,7 +351,8 @@ struct svc_deferred_req {
> struct svc_xprt *xprt;
> struct sockaddr_storage addr; /* where reply must go */
> size_t addrlen;
> - union svc_addr_u daddr; /* where reply must come from */
> + struct sockaddr_storage daddr; /* where reply must come from */
> + size_t daddrlen;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..d86bb67 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
> */
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> {
> - struct sockaddr *sin;
> -
> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
> rqstp->rq_addrlen = xprt->xpt_remotelen;
>
> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * Destination address in request is needed for binding the
> * source address in RPC replies/callbacks later.
> */
> - sin = (struct sockaddr *)&xprt->xpt_local;
> - switch (sin->sa_family) {
> - case AF_INET:
> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> - break;
> - case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> - break;
> - }
> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
> + rqstp->rq_daddrlen = xprt->xpt_locallen;
> }
> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..b9f4ad4 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_level = SOL_IP;
> cmh->cmsg_type = IP_PKTINFO;
> pki->ipi_ifindex = 0;
> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
> + pki->ipi_spec_dst.s_addr =
> + svc_daddr_in(rqstp)->sin_addr.s_addr;
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_type = IPV6_PKTINFO;
> pki->ipi6_ifindex = 0;
> ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + &svc_daddr_in6(rqstp)->sin6_addr);
I kind of suspect more is needed here to deal with ipi6_ifindex.
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> struct in_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IP_PKTINFO)
> return 0;
> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> return 1;
> }
>
> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
Likewise, is the incoming interface index ignored when constructing svc_daddr_in6(rqstp) ?
> return 1;
> }
>
> --
> 1.7.6
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-25 14:30 ` Chuck Lever
@ 2011-08-25 14:37 ` Chuck Lever
2011-08-26 6:58 ` Mi Jinlong
0 siblings, 1 reply; 24+ messages in thread
From: Chuck Lever @ 2011-08-25 14:37 UTC (permalink / raw)
To: Mi Jinlong; +Cc: J. Bruce Fields, NFS, Trond Myklebust
On Aug 25, 2011, at 10:30 AM, Chuck Lever wrote:
> This seems like a good direction to go in. More comments below.
>
> On Aug 24, 2011, at 1:58 AM, Mi Jinlong wrote:
>
>> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> Date: Mon, 24 Aug 2011 13:22:39 +0800
>> Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
>>
>> For IPv6 local address, lockd can not callback to client for
>> missing scope id when binding address at inet6_bind:
>>
>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>> 326 addr->sin6_scope_id) {
>> 327 /* Override any existing binding, if another one
>> 328 * is supplied by user.
>> 329 */
>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>> 331 }
>> 332
>> 333 /* Binding to link-local address requires an interface */
>> 334 if (!sk->sk_bound_dev_if) {
>> 335 err = -EINVAL;
>> 336 goto out_unlock;
>> 337 }
>>
>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
>> besides address.
>>
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>> fs/lockd/host.c | 14 ++------------
>> fs/nfsd/nfs4state.c | 16 +---------------
>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
>> net/sunrpc/svc_xprt.c | 13 ++-----------
>> net/sunrpc/svcsock.c | 9 +++++----
>> 5 files changed, 31 insertions(+), 51 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index b7c99bf..b23db05 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>> struct hlist_node *pos;
>> struct nlm_host *host = NULL;
>> struct nsm_handle *nsm = NULL;
>> - struct sockaddr_in sin = {
>> - .sin_family = AF_INET,
>> - };
>> - struct sockaddr_in6 sin6 = {
>> - .sin6_family = AF_INET6,
>> - };
>> struct sockaddr *src_sap;
>> - size_t src_len = rqstp->rq_addrlen;
>> + size_t src_len = rqstp->rq_daddrlen;
>> struct nlm_lookup_host_info ni = {
>> .server = 1,
>> .sap = svc_addr(rqstp),
>> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>
>> switch (ni.sap->sa_family) {
>> case AF_INET:
>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
>> - src_sap = (struct sockaddr *)&sin;
>> - break;
>> case AF_INET6:
>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
>> - src_sap = (struct sockaddr *)&sin6;
>> + src_sap = svc_daddr(rqstp);
>> break;
>> default:
>> dprintk("lockd: %s failed; unrecognized address family\n",
>
> With the destination address promoted to a full sockaddr, we probably do not need this switch any more.
>
> I notice that the IPv6 fork of rpc_cmp_addr() does not consider the scope IDs of the passed-in addresses. Perhaps it should?
>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 3787ec1..07a18a5 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>> return NULL;
>> }
>>
>> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
>> -{
>> - switch (family) {
>> - case AF_INET:
>> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
>> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
>> - return;
>> - case AF_INET6:
>> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
>> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
>> - return;
>> - }
>> -}
>> -
>> static void
>> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
>> {
>> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>>
>> conn->cb_prog = se->se_callback_prog;
>> conn->cb_ident = se->se_callback_ident;
>> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
>> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
>> return;
>> out_err:
>> conn->cb_addr.ss_family = AF_UNSPEC;
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 223588a..3bc0dc9 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
>> iov->iov_len += sizeof(__be32);
>> }
>>
>> -union svc_addr_u {
>> - struct in_addr addr;
>> - struct in6_addr addr6;
>> -};
>> -
>> /*
>> * The context of a single thread, including the request currently being
>> * processed.
>> @@ -225,8 +220,12 @@ struct svc_rqst {
>> struct list_head rq_list; /* idle list */
>> struct list_head rq_all; /* all threads list */
>> struct svc_xprt * rq_xprt; /* transport ptr */
>> +
>> struct sockaddr_storage rq_addr; /* peer address */
>> size_t rq_addrlen;
>> + struct sockaddr_storage rq_daddr; /* dest addr of request
>> + * - reply from here */
>> + size_t rq_daddrlen;
>>
>> struct svc_serv * rq_server; /* RPC service definition */
>> struct svc_pool * rq_pool; /* thread pool */
>> @@ -255,9 +254,6 @@ struct svc_rqst {
>> unsigned short
>> rq_secure : 1; /* secure port */
>>
>> - union svc_addr_u rq_daddr; /* dest addr of request
>> - * - reply from here */
>> -
>> void * rq_argp; /* decoded arguments */
>> void * rq_resp; /* xdr'd results */
>> void * rq_auth_data; /* flavor-specific data */
>> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
>> return (struct sockaddr *) &rqst->rq_addr;
>> }
>>
>> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr *) &rqst->rq_daddr;
>> +}
>> +
>> /*
>> * Check buffer bounds after decoding arguments
>> */
>> @@ -340,7 +351,8 @@ struct svc_deferred_req {
>> struct svc_xprt *xprt;
>> struct sockaddr_storage addr; /* where reply must go */
>> size_t addrlen;
>> - union svc_addr_u daddr; /* where reply must come from */
>> + struct sockaddr_storage daddr; /* where reply must come from */
>> + size_t daddrlen;
>> struct cache_deferred_req handle;
>> size_t xprt_hlen;
>> int argslen;
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index bd31208..d86bb67 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
>> */
>> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> {
>> - struct sockaddr *sin;
>> -
>> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
>> rqstp->rq_addrlen = xprt->xpt_remotelen;
>>
>> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> * Destination address in request is needed for binding the
>> * source address in RPC replies/callbacks later.
>> */
>> - sin = (struct sockaddr *)&xprt->xpt_local;
>> - switch (sin->sa_family) {
>> - case AF_INET:
>> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
>> - break;
>> - case AF_INET6:
>> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
>> - break;
>> - }
>> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
>> + rqstp->rq_daddrlen = xprt->xpt_locallen;
>> }
>> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 767d494..b9f4ad4 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>> cmh->cmsg_level = SOL_IP;
>> cmh->cmsg_type = IP_PKTINFO;
>> pki->ipi_ifindex = 0;
>> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
>> + pki->ipi_spec_dst.s_addr =
>> + svc_daddr_in(rqstp)->sin_addr.s_addr;
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>> cmh->cmsg_type = IPV6_PKTINFO;
>> pki->ipi6_ifindex = 0;
>> ipv6_addr_copy(&pki->ipi6_addr,
>> - &rqstp->rq_daddr.addr6);
>> + &svc_daddr_in6(rqstp)->sin6_addr);
>
> I kind of suspect more is needed here to deal with ipi6_ifindex.
>
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
>> struct in_pktinfo *pki = CMSG_DATA(cmh);
>> if (cmh->cmsg_type != IP_PKTINFO)
>> return 0;
>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>> return 1;
>> }
>>
>> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> if (cmh->cmsg_type != IPV6_PKTINFO)
>> return 0;
>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
>
> Likewise, is the incoming interface index ignored when constructing svc_daddr_in6(rqstp) ?
Also, I can't seem to find where the address family field of svc_daddr() is being set.
>
>> return 1;
>> }
>>
>> --
>> 1.7.6
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-25 14:37 ` Chuck Lever
@ 2011-08-26 6:58 ` Mi Jinlong
2011-08-26 22:43 ` J. Bruce Fields
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Mi Jinlong @ 2011-08-26 6:58 UTC (permalink / raw)
To: Chuck Lever; +Cc: J. Bruce Fields, NFS, Trond Myklebust
Thanks for you comment!
Chuck Lever:
> On Aug 25, 2011, at 10:30 AM, Chuck Lever wrote:
>
>> This seems like a good direction to go in. More comments below.
>>
>> On Aug 24, 2011, at 1:58 AM, Mi Jinlong wrote:
>>
>>> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>> Date: Mon, 24 Aug 2011 13:22:39 +0800
>>> Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
>>>
>>> For IPv6 local address, lockd can not callback to client for
>>> missing scope id when binding address at inet6_bind:
>>>
>>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>>> 326 addr->sin6_scope_id) {
>>> 327 /* Override any existing binding, if another one
>>> 328 * is supplied by user.
>>> 329 */
>>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>>> 331 }
>>> 332
>>> 333 /* Binding to link-local address requires an interface */
>>> 334 if (!sk->sk_bound_dev_if) {
>>> 335 err = -EINVAL;
>>> 336 goto out_unlock;
>>> 337 }
>>>
>>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
>>> besides address.
>>>
>>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>> ---
>>> fs/lockd/host.c | 14 ++------------
>>> fs/nfsd/nfs4state.c | 16 +---------------
>>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
>>> net/sunrpc/svc_xprt.c | 13 ++-----------
>>> net/sunrpc/svcsock.c | 9 +++++----
>>> 5 files changed, 31 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>>> index b7c99bf..b23db05 100644
>>> --- a/fs/lockd/host.c
>>> +++ b/fs/lockd/host.c
>>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>> struct hlist_node *pos;
>>> struct nlm_host *host = NULL;
>>> struct nsm_handle *nsm = NULL;
>>> - struct sockaddr_in sin = {
>>> - .sin_family = AF_INET,
>>> - };
>>> - struct sockaddr_in6 sin6 = {
>>> - .sin6_family = AF_INET6,
>>> - };
>>> struct sockaddr *src_sap;
>>> - size_t src_len = rqstp->rq_addrlen;
>>> + size_t src_len = rqstp->rq_daddrlen;
>>> struct nlm_lookup_host_info ni = {
>>> .server = 1,
>>> .sap = svc_addr(rqstp),
>>> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>>
>>> switch (ni.sap->sa_family) {
>>> case AF_INET:
>>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
>>> - src_sap = (struct sockaddr *)&sin;
>>> - break;
>>> case AF_INET6:
>>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
>>> - src_sap = (struct sockaddr *)&sin6;
>>> + src_sap = svc_daddr(rqstp);
>>> break;
>>> default:
>>> dprintk("lockd: %s failed; unrecognized address family\n",
>> With the destination address promoted to a full sockaddr, we probably do not need this switch any more.
Got it.
>>
>> I notice that the IPv6 fork of rpc_cmp_addr() does not consider the scope IDs of the passed-in addresses. Perhaps it should?
Have add scope ID compare at __rpc_cmp_addr6 as following patch.
>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 3787ec1..07a18a5 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>>> return NULL;
>>> }
... snip ...
>>> break;
>>> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>>> cmh->cmsg_type = IPV6_PKTINFO;
>>> pki->ipi6_ifindex = 0;
>>> ipv6_addr_copy(&pki->ipi6_addr,
>>> - &rqstp->rq_daddr.addr6);
>>> + &svc_daddr_in6(rqstp)->sin6_addr);
>> I kind of suspect more is needed here to deal with ipi6_ifindex.
Got it.
>>
>>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>>> }
>>> break;
>>> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
>>> struct in_pktinfo *pki = CMSG_DATA(cmh);
>>> if (cmh->cmsg_type != IP_PKTINFO)
>>> return 0;
>>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>>> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>>> return 1;
>>> }
>>>
>>> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
>>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>>> if (cmh->cmsg_type != IPV6_PKTINFO)
>>> return 0;
>>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>>> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
>> Likewise, is the incoming interface index ignored when constructing svc_daddr_in6(rqstp) ?
>
> Also, I can't seem to find where the address family field of svc_daddr() is being set.
>
Have complete it.
thanks,
Mi Jinlong
>From 84420475509ccc39ac8899f6f63140879a5278b6 Mon Sep 17 00:00:00 2001
From: Mi Jinlong <mijinlong@cn.fujitsu.com>
Date: Mon, 26 Aug 2011 13:22:39 +0800
Subject: [PATCH v2] SUNRPC: Replace svc_addr_u by sockaddr_storage
For IPv6 local address, lockd can not callback to client for
missing scope id when binding address at inet6_bind:
324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
325 if (addr_len >= sizeof(struct sockaddr_in6) &&
326 addr->sin6_scope_id) {
327 /* Override any existing binding, if another one
328 * is supplied by user.
329 */
330 sk->sk_bound_dev_if = addr->sin6_scope_id;
331 }
332
333 /* Binding to link-local address requires an interface */
334 if (!sk->sk_bound_dev_if) {
335 err = -EINVAL;
336 goto out_unlock;
337 }
Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
besides address.
Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
fs/lockd/host.c | 25 ++-----------------------
fs/nfsd/nfs4state.c | 16 +---------------
include/linux/sunrpc/clnt.h | 3 ++-
include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
net/sunrpc/svc_xprt.c | 13 ++-----------
net/sunrpc/svcsock.c | 23 +++++++++++++++++------
6 files changed, 45 insertions(+), 65 deletions(-)
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index b7c99bf..6f29836 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
struct hlist_node *pos;
struct nlm_host *host = NULL;
struct nsm_handle *nsm = NULL;
- struct sockaddr_in sin = {
- .sin_family = AF_INET,
- };
- struct sockaddr_in6 sin6 = {
- .sin6_family = AF_INET6,
- };
- struct sockaddr *src_sap;
- size_t src_len = rqstp->rq_addrlen;
+ struct sockaddr *src_sap = svc_daddr(rqstp);
+ size_t src_len = rqstp->rq_daddrlen;
struct nlm_lookup_host_info ni = {
.server = 1,
.sap = svc_addr(rqstp),
@@ -340,21 +334,6 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
mutex_lock(&nlm_host_mutex);
- switch (ni.sap->sa_family) {
- case AF_INET:
- sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
- src_sap = (struct sockaddr *)&sin;
- break;
- case AF_INET6:
- ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
- src_sap = (struct sockaddr *)&sin6;
- break;
- default:
- dprintk("lockd: %s failed; unrecognized address family\n",
- __func__);
- goto out;
- }
-
if (time_after_eq(jiffies, next_gc))
nlm_gc_hosts();
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3787ec1..07a18a5 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
return NULL;
}
-static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
-{
- switch (family) {
- case AF_INET:
- ((struct sockaddr_in *)sa)->sin_family = AF_INET;
- ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
- return;
- case AF_INET6:
- ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
- ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
- return;
- }
-}
-
static void
gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
{
@@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
conn->cb_prog = se->se_callback_prog;
conn->cb_ident = se->se_callback_ident;
- rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
+ memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
return;
out_err:
conn->cb_addr.ss_family = AF_UNSPEC;
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index db7bcaf..1529a80 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -218,7 +218,8 @@ static inline bool __rpc_cmp_addr6(const struct sockaddr *sap1,
{
const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sap1;
const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sap2;
- return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr);
+ return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr)
+ && (sin1->sin6_scope_id == sin2->sin6_scope_id);
}
static inline bool __rpc_copy_addr6(struct sockaddr *dst,
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 223588a..3bc0dc9 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
iov->iov_len += sizeof(__be32);
}
-union svc_addr_u {
- struct in_addr addr;
- struct in6_addr addr6;
-};
-
/*
* The context of a single thread, including the request currently being
* processed.
@@ -225,8 +220,12 @@ struct svc_rqst {
struct list_head rq_list; /* idle list */
struct list_head rq_all; /* all threads list */
struct svc_xprt * rq_xprt; /* transport ptr */
+
struct sockaddr_storage rq_addr; /* peer address */
size_t rq_addrlen;
+ struct sockaddr_storage rq_daddr; /* dest addr of request
+ * - reply from here */
+ size_t rq_daddrlen;
struct svc_serv * rq_server; /* RPC service definition */
struct svc_pool * rq_pool; /* thread pool */
@@ -255,9 +254,6 @@ struct svc_rqst {
unsigned short
rq_secure : 1; /* secure port */
- union svc_addr_u rq_daddr; /* dest addr of request
- * - reply from here */
-
void * rq_argp; /* decoded arguments */
void * rq_resp; /* xdr'd results */
void * rq_auth_data; /* flavor-specific data */
@@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
return (struct sockaddr *) &rqst->rq_addr;
}
+static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
+{
+ return (struct sockaddr_in *) &rqst->rq_daddr;
+}
+
+static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
+{
+ return (struct sockaddr_in6 *) &rqst->rq_daddr;
+}
+
+static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
+{
+ return (struct sockaddr *) &rqst->rq_daddr;
+}
+
/*
* Check buffer bounds after decoding arguments
*/
@@ -340,7 +351,8 @@ struct svc_deferred_req {
struct svc_xprt *xprt;
struct sockaddr_storage addr; /* where reply must go */
size_t addrlen;
- union svc_addr_u daddr; /* where reply must come from */
+ struct sockaddr_storage daddr; /* where reply must come from */
+ size_t daddrlen;
struct cache_deferred_req handle;
size_t xprt_hlen;
int argslen;
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index bd31208..d86bb67 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
*/
void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
{
- struct sockaddr *sin;
-
memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
rqstp->rq_addrlen = xprt->xpt_remotelen;
@@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
* Destination address in request is needed for binding the
* source address in RPC replies/callbacks later.
*/
- sin = (struct sockaddr *)&xprt->xpt_local;
- switch (sin->sa_family) {
- case AF_INET:
- rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
- break;
- case AF_INET6:
- rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
- break;
- }
+ memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
+ rqstp->rq_daddrlen = xprt->xpt_locallen;
}
EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 767d494..dfd686e 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -143,19 +143,20 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
cmh->cmsg_level = SOL_IP;
cmh->cmsg_type = IP_PKTINFO;
pki->ipi_ifindex = 0;
- pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
+ pki->ipi_spec_dst.s_addr =
+ svc_daddr_in(rqstp)->sin_addr.s_addr;
cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
}
break;
case AF_INET6: {
struct in6_pktinfo *pki = CMSG_DATA(cmh);
+ struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
cmh->cmsg_level = SOL_IPV6;
cmh->cmsg_type = IPV6_PKTINFO;
- pki->ipi6_ifindex = 0;
- ipv6_addr_copy(&pki->ipi6_addr,
- &rqstp->rq_daddr.addr6);
+ pki->ipi6_ifindex = daddr->sin6_scope_id;
+ ipv6_addr_copy(&pki->ipi6_addr, &daddr->sin6_addr);
cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
}
break;
@@ -498,9 +499,13 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
struct cmsghdr *cmh)
{
struct in_pktinfo *pki = CMSG_DATA(cmh);
+ struct sockaddr_in *daddr = svc_daddr_in(rqstp);
+
if (cmh->cmsg_type != IP_PKTINFO)
return 0;
- rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
+
+ daddr->sin_family = AF_INET;
+ daddr->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
return 1;
}
@@ -511,9 +516,14 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
struct cmsghdr *cmh)
{
struct in6_pktinfo *pki = CMSG_DATA(cmh);
+ struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
+
if (cmh->cmsg_type != IPV6_PKTINFO)
return 0;
- ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
+
+ daddr->sin6_family = AF_INET6;
+ ipv6_addr_copy(&daddr->sin6_addr, &pki->ipi6_addr);
+ daddr->sin6_scope_id = pki->ipi6_ifindex;
return 1;
}
@@ -614,6 +624,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
skb_free_datagram_locked(svsk->sk_sk, skb);
return 0;
}
+ rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
if (skb_is_nonlinear(skb)) {
/* we have to copy */
--
1.7.6
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-26 6:58 ` Mi Jinlong
@ 2011-08-26 22:43 ` J. Bruce Fields
2011-08-27 22:03 ` Chuck Lever
2011-08-29 15:26 ` Jeff Layton
2011-08-29 17:09 ` Steve Dickson
2 siblings, 1 reply; 24+ messages in thread
From: J. Bruce Fields @ 2011-08-26 22:43 UTC (permalink / raw)
To: Mi Jinlong; +Cc: Chuck Lever, NFS, Trond Myklebust, Jeff Layton
Jeff and Chuck, you're OK with the revised version?
--b.
On Fri, Aug 26, 2011 at 02:58:23PM +0800, Mi Jinlong wrote:
> >From 84420475509ccc39ac8899f6f63140879a5278b6 Mon Sep 17 00:00:00 2001
> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> Date: Mon, 26 Aug 2011 13:22:39 +0800
> Subject: [PATCH v2] SUNRPC: Replace svc_addr_u by sockaddr_storage
>
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> besides address.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 25 ++-----------------------
> fs/nfsd/nfs4state.c | 16 +---------------
> include/linux/sunrpc/clnt.h | 3 ++-
> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> net/sunrpc/svc_xprt.c | 13 ++-----------
> net/sunrpc/svcsock.c | 23 +++++++++++++++++------
> 6 files changed, 45 insertions(+), 65 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..6f29836 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> struct hlist_node *pos;
> struct nlm_host *host = NULL;
> struct nsm_handle *nsm = NULL;
> - struct sockaddr_in sin = {
> - .sin_family = AF_INET,
> - };
> - struct sockaddr_in6 sin6 = {
> - .sin6_family = AF_INET6,
> - };
> - struct sockaddr *src_sap;
> - size_t src_len = rqstp->rq_addrlen;
> + struct sockaddr *src_sap = svc_daddr(rqstp);
> + size_t src_len = rqstp->rq_daddrlen;
> struct nlm_lookup_host_info ni = {
> .server = 1,
> .sap = svc_addr(rqstp),
> @@ -340,21 +334,6 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>
> mutex_lock(&nlm_host_mutex);
>
> - switch (ni.sap->sa_family) {
> - case AF_INET:
> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> - src_sap = (struct sockaddr *)&sin;
> - break;
> - case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> - src_sap = (struct sockaddr *)&sin6;
> - break;
> - default:
> - dprintk("lockd: %s failed; unrecognized address family\n",
> - __func__);
> - goto out;
> - }
> -
> if (time_after_eq(jiffies, next_gc))
> nlm_gc_hosts();
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..07a18a5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> return NULL;
> }
>
> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
> -{
> - switch (family) {
> - case AF_INET:
> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
> - return;
> - case AF_INET6:
> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> - return;
> - }
> -}
> -
> static void
> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
> {
> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index db7bcaf..1529a80 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -218,7 +218,8 @@ static inline bool __rpc_cmp_addr6(const struct sockaddr *sap1,
> {
> const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sap1;
> const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sap2;
> - return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr);
> + return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr)
> + && (sin1->sin6_scope_id == sin2->sin6_scope_id);
> }
>
> static inline bool __rpc_copy_addr6(struct sockaddr *dst,
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..3bc0dc9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> -union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> -};
> -
> /*
> * The context of a single thread, including the request currently being
> * processed.
> @@ -225,8 +220,12 @@ struct svc_rqst {
> struct list_head rq_list; /* idle list */
> struct list_head rq_all; /* all threads list */
> struct svc_xprt * rq_xprt; /* transport ptr */
> +
> struct sockaddr_storage rq_addr; /* peer address */
> size_t rq_addrlen;
> + struct sockaddr_storage rq_daddr; /* dest addr of request
> + * - reply from here */
> + size_t rq_daddrlen;
>
> struct svc_serv * rq_server; /* RPC service definition */
> struct svc_pool * rq_pool; /* thread pool */
> @@ -255,9 +254,6 @@ struct svc_rqst {
> unsigned short
> rq_secure : 1; /* secure port */
>
> - union svc_addr_u rq_daddr; /* dest addr of request
> - * - reply from here */
> -
> void * rq_argp; /* decoded arguments */
> void * rq_resp; /* xdr'd results */
> void * rq_auth_data; /* flavor-specific data */
> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_addr;
> }
>
> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr *) &rqst->rq_daddr;
> +}
> +
> /*
> * Check buffer bounds after decoding arguments
> */
> @@ -340,7 +351,8 @@ struct svc_deferred_req {
> struct svc_xprt *xprt;
> struct sockaddr_storage addr; /* where reply must go */
> size_t addrlen;
> - union svc_addr_u daddr; /* where reply must come from */
> + struct sockaddr_storage daddr; /* where reply must come from */
> + size_t daddrlen;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..d86bb67 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
> */
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> {
> - struct sockaddr *sin;
> -
> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
> rqstp->rq_addrlen = xprt->xpt_remotelen;
>
> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * Destination address in request is needed for binding the
> * source address in RPC replies/callbacks later.
> */
> - sin = (struct sockaddr *)&xprt->xpt_local;
> - switch (sin->sa_family) {
> - case AF_INET:
> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> - break;
> - case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> - break;
> - }
> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
> + rqstp->rq_daddrlen = xprt->xpt_locallen;
> }
> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..dfd686e 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -143,19 +143,20 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_level = SOL_IP;
> cmh->cmsg_type = IP_PKTINFO;
> pki->ipi_ifindex = 0;
> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
> + pki->ipi_spec_dst.s_addr =
> + svc_daddr_in(rqstp)->sin_addr.s_addr;
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
>
> case AF_INET6: {
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>
> cmh->cmsg_level = SOL_IPV6;
> cmh->cmsg_type = IPV6_PKTINFO;
> - pki->ipi6_ifindex = 0;
> - ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + pki->ipi6_ifindex = daddr->sin6_scope_id;
> + ipv6_addr_copy(&pki->ipi6_addr, &daddr->sin6_addr);
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -498,9 +499,13 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> struct cmsghdr *cmh)
> {
> struct in_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in *daddr = svc_daddr_in(rqstp);
> +
> if (cmh->cmsg_type != IP_PKTINFO)
> return 0;
> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> +
> + daddr->sin_family = AF_INET;
> + daddr->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> return 1;
> }
>
> @@ -511,9 +516,14 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct cmsghdr *cmh)
> {
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
> +
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> +
> + daddr->sin6_family = AF_INET6;
> + ipv6_addr_copy(&daddr->sin6_addr, &pki->ipi6_addr);
> + daddr->sin6_scope_id = pki->ipi6_ifindex;
> return 1;
> }
>
> @@ -614,6 +624,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> skb_free_datagram_locked(svsk->sk_sk, skb);
> return 0;
> }
> + rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
>
> if (skb_is_nonlinear(skb)) {
> /* we have to copy */
> --
> 1.7.6
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-26 22:43 ` J. Bruce Fields
@ 2011-08-27 22:03 ` Chuck Lever
0 siblings, 0 replies; 24+ messages in thread
From: Chuck Lever @ 2011-08-27 22:03 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Mi Jinlong, NFS, Trond Myklebust, Jeff Layton
I don't see any obvious problems with it. It would be nice to see this patch in operation on an IPv6-capable network.
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
On Aug 26, 2011, at 6:43 PM, J. Bruce Fields wrote:
> Jeff and Chuck, you're OK with the revised version?
>
> --b.
>
> On Fri, Aug 26, 2011 at 02:58:23PM +0800, Mi Jinlong wrote:
>>> From 84420475509ccc39ac8899f6f63140879a5278b6 Mon Sep 17 00:00:00 2001
>> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> Date: Mon, 26 Aug 2011 13:22:39 +0800
>> Subject: [PATCH v2] SUNRPC: Replace svc_addr_u by sockaddr_storage
>>
>> For IPv6 local address, lockd can not callback to client for
>> missing scope id when binding address at inet6_bind:
>>
>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>> 326 addr->sin6_scope_id) {
>> 327 /* Override any existing binding, if another one
>> 328 * is supplied by user.
>> 329 */
>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>> 331 }
>> 332
>> 333 /* Binding to link-local address requires an interface */
>> 334 if (!sk->sk_bound_dev_if) {
>> 335 err = -EINVAL;
>> 336 goto out_unlock;
>> 337 }
>>
>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
>> besides address.
>>
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>> fs/lockd/host.c | 25 ++-----------------------
>> fs/nfsd/nfs4state.c | 16 +---------------
>> include/linux/sunrpc/clnt.h | 3 ++-
>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
>> net/sunrpc/svc_xprt.c | 13 ++-----------
>> net/sunrpc/svcsock.c | 23 +++++++++++++++++------
>> 6 files changed, 45 insertions(+), 65 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index b7c99bf..6f29836 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>> struct hlist_node *pos;
>> struct nlm_host *host = NULL;
>> struct nsm_handle *nsm = NULL;
>> - struct sockaddr_in sin = {
>> - .sin_family = AF_INET,
>> - };
>> - struct sockaddr_in6 sin6 = {
>> - .sin6_family = AF_INET6,
>> - };
>> - struct sockaddr *src_sap;
>> - size_t src_len = rqstp->rq_addrlen;
>> + struct sockaddr *src_sap = svc_daddr(rqstp);
>> + size_t src_len = rqstp->rq_daddrlen;
>> struct nlm_lookup_host_info ni = {
>> .server = 1,
>> .sap = svc_addr(rqstp),
>> @@ -340,21 +334,6 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>
>> mutex_lock(&nlm_host_mutex);
>>
>> - switch (ni.sap->sa_family) {
>> - case AF_INET:
>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
>> - src_sap = (struct sockaddr *)&sin;
>> - break;
>> - case AF_INET6:
>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
>> - src_sap = (struct sockaddr *)&sin6;
>> - break;
>> - default:
>> - dprintk("lockd: %s failed; unrecognized address family\n",
>> - __func__);
>> - goto out;
>> - }
>> -
>> if (time_after_eq(jiffies, next_gc))
>> nlm_gc_hosts();
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 3787ec1..07a18a5 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>> return NULL;
>> }
>>
>> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
>> -{
>> - switch (family) {
>> - case AF_INET:
>> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
>> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
>> - return;
>> - case AF_INET6:
>> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
>> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
>> - return;
>> - }
>> -}
>> -
>> static void
>> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
>> {
>> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>>
>> conn->cb_prog = se->se_callback_prog;
>> conn->cb_ident = se->se_callback_ident;
>> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
>> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
>> return;
>> out_err:
>> conn->cb_addr.ss_family = AF_UNSPEC;
>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>> index db7bcaf..1529a80 100644
>> --- a/include/linux/sunrpc/clnt.h
>> +++ b/include/linux/sunrpc/clnt.h
>> @@ -218,7 +218,8 @@ static inline bool __rpc_cmp_addr6(const struct sockaddr *sap1,
>> {
>> const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sap1;
>> const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sap2;
>> - return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr);
>> + return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr)
>> + && (sin1->sin6_scope_id == sin2->sin6_scope_id);
>> }
>>
>> static inline bool __rpc_copy_addr6(struct sockaddr *dst,
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 223588a..3bc0dc9 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
>> iov->iov_len += sizeof(__be32);
>> }
>>
>> -union svc_addr_u {
>> - struct in_addr addr;
>> - struct in6_addr addr6;
>> -};
>> -
>> /*
>> * The context of a single thread, including the request currently being
>> * processed.
>> @@ -225,8 +220,12 @@ struct svc_rqst {
>> struct list_head rq_list; /* idle list */
>> struct list_head rq_all; /* all threads list */
>> struct svc_xprt * rq_xprt; /* transport ptr */
>> +
>> struct sockaddr_storage rq_addr; /* peer address */
>> size_t rq_addrlen;
>> + struct sockaddr_storage rq_daddr; /* dest addr of request
>> + * - reply from here */
>> + size_t rq_daddrlen;
>>
>> struct svc_serv * rq_server; /* RPC service definition */
>> struct svc_pool * rq_pool; /* thread pool */
>> @@ -255,9 +254,6 @@ struct svc_rqst {
>> unsigned short
>> rq_secure : 1; /* secure port */
>>
>> - union svc_addr_u rq_daddr; /* dest addr of request
>> - * - reply from here */
>> -
>> void * rq_argp; /* decoded arguments */
>> void * rq_resp; /* xdr'd results */
>> void * rq_auth_data; /* flavor-specific data */
>> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
>> return (struct sockaddr *) &rqst->rq_addr;
>> }
>>
>> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr *) &rqst->rq_daddr;
>> +}
>> +
>> /*
>> * Check buffer bounds after decoding arguments
>> */
>> @@ -340,7 +351,8 @@ struct svc_deferred_req {
>> struct svc_xprt *xprt;
>> struct sockaddr_storage addr; /* where reply must go */
>> size_t addrlen;
>> - union svc_addr_u daddr; /* where reply must come from */
>> + struct sockaddr_storage daddr; /* where reply must come from */
>> + size_t daddrlen;
>> struct cache_deferred_req handle;
>> size_t xprt_hlen;
>> int argslen;
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index bd31208..d86bb67 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
>> */
>> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> {
>> - struct sockaddr *sin;
>> -
>> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
>> rqstp->rq_addrlen = xprt->xpt_remotelen;
>>
>> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> * Destination address in request is needed for binding the
>> * source address in RPC replies/callbacks later.
>> */
>> - sin = (struct sockaddr *)&xprt->xpt_local;
>> - switch (sin->sa_family) {
>> - case AF_INET:
>> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
>> - break;
>> - case AF_INET6:
>> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
>> - break;
>> - }
>> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
>> + rqstp->rq_daddrlen = xprt->xpt_locallen;
>> }
>> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 767d494..dfd686e 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -143,19 +143,20 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>> cmh->cmsg_level = SOL_IP;
>> cmh->cmsg_type = IP_PKTINFO;
>> pki->ipi_ifindex = 0;
>> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
>> + pki->ipi_spec_dst.s_addr =
>> + svc_daddr_in(rqstp)->sin_addr.s_addr;
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>>
>> case AF_INET6: {
>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>>
>> cmh->cmsg_level = SOL_IPV6;
>> cmh->cmsg_type = IPV6_PKTINFO;
>> - pki->ipi6_ifindex = 0;
>> - ipv6_addr_copy(&pki->ipi6_addr,
>> - &rqstp->rq_daddr.addr6);
>> + pki->ipi6_ifindex = daddr->sin6_scope_id;
>> + ipv6_addr_copy(&pki->ipi6_addr, &daddr->sin6_addr);
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>> @@ -498,9 +499,13 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
>> struct cmsghdr *cmh)
>> {
>> struct in_pktinfo *pki = CMSG_DATA(cmh);
>> + struct sockaddr_in *daddr = svc_daddr_in(rqstp);
>> +
>> if (cmh->cmsg_type != IP_PKTINFO)
>> return 0;
>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>> +
>> + daddr->sin_family = AF_INET;
>> + daddr->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>> return 1;
>> }
>>
>> @@ -511,9 +516,14 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
>> struct cmsghdr *cmh)
>> {
>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>> +
>> if (cmh->cmsg_type != IPV6_PKTINFO)
>> return 0;
>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>> +
>> + daddr->sin6_family = AF_INET6;
>> + ipv6_addr_copy(&daddr->sin6_addr, &pki->ipi6_addr);
>> + daddr->sin6_scope_id = pki->ipi6_ifindex;
>> return 1;
>> }
>>
>> @@ -614,6 +624,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>> skb_free_datagram_locked(svsk->sk_sk, skb);
>> return 0;
>> }
>> + rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
>>
>> if (skb_is_nonlinear(skb)) {
>> /* we have to copy */
>> --
>> 1.7.6
>>
>>
>>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-26 6:58 ` Mi Jinlong
2011-08-26 22:43 ` J. Bruce Fields
@ 2011-08-29 15:26 ` Jeff Layton
2011-08-29 15:30 ` Chuck Lever
2011-08-29 17:09 ` Steve Dickson
2 siblings, 1 reply; 24+ messages in thread
From: Jeff Layton @ 2011-08-29 15:26 UTC (permalink / raw)
To: Mi Jinlong; +Cc: Chuck Lever, J. Bruce Fields, NFS, Trond Myklebust
On Fri, 26 Aug 2011 14:58:23 +0800
Mi Jinlong <mijinlong@cn.fujitsu.com> wrote:
> Thanks for you comment!
>
> Chuck Lever:
> > On Aug 25, 2011, at 10:30 AM, Chuck Lever wrote:
> >
> >> This seems like a good direction to go in. More comments below.
> >>
> >> On Aug 24, 2011, at 1:58 AM, Mi Jinlong wrote:
> >>
> >>> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> >>> Date: Mon, 24 Aug 2011 13:22:39 +0800
> >>> Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
> >>>
> >>> For IPv6 local address, lockd can not callback to client for
> >>> missing scope id when binding address at inet6_bind:
> >>>
> >>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> >>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> >>> 326 addr->sin6_scope_id) {
> >>> 327 /* Override any existing binding, if another one
> >>> 328 * is supplied by user.
> >>> 329 */
> >>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> >>> 331 }
> >>> 332
> >>> 333 /* Binding to link-local address requires an interface */
> >>> 334 if (!sk->sk_bound_dev_if) {
> >>> 335 err = -EINVAL;
> >>> 336 goto out_unlock;
> >>> 337 }
> >>>
> >>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> >>> besides address.
> >>>
> >>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> >>> ---
> >>> fs/lockd/host.c | 14 ++------------
> >>> fs/nfsd/nfs4state.c | 16 +---------------
> >>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> >>> net/sunrpc/svc_xprt.c | 13 ++-----------
> >>> net/sunrpc/svcsock.c | 9 +++++----
> >>> 5 files changed, 31 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> >>> index b7c99bf..b23db05 100644
> >>> --- a/fs/lockd/host.c
> >>> +++ b/fs/lockd/host.c
> >>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> >>> struct hlist_node *pos;
> >>> struct nlm_host *host = NULL;
> >>> struct nsm_handle *nsm = NULL;
> >>> - struct sockaddr_in sin = {
> >>> - .sin_family = AF_INET,
> >>> - };
> >>> - struct sockaddr_in6 sin6 = {
> >>> - .sin6_family = AF_INET6,
> >>> - };
> >>> struct sockaddr *src_sap;
> >>> - size_t src_len = rqstp->rq_addrlen;
> >>> + size_t src_len = rqstp->rq_daddrlen;
> >>> struct nlm_lookup_host_info ni = {
> >>> .server = 1,
> >>> .sap = svc_addr(rqstp),
> >>> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> >>>
> >>> switch (ni.sap->sa_family) {
> >>> case AF_INET:
> >>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> >>> - src_sap = (struct sockaddr *)&sin;
> >>> - break;
> >>> case AF_INET6:
> >>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> >>> - src_sap = (struct sockaddr *)&sin6;
> >>> + src_sap = svc_daddr(rqstp);
> >>> break;
> >>> default:
> >>> dprintk("lockd: %s failed; unrecognized address family\n",
> >> With the destination address promoted to a full sockaddr, we probably do not need this switch any more.
>
> Got it.
>
> >>
> >> I notice that the IPv6 fork of rpc_cmp_addr() does not consider the scope IDs of the passed-in addresses. Perhaps it should?
>
> Have add scope ID compare at __rpc_cmp_addr6 as following patch.
>
> >>
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index 3787ec1..07a18a5 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> >>> return NULL;
> >>> }
> ... snip ...
> >>> break;
> >>> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> >>> cmh->cmsg_type = IPV6_PKTINFO;
> >>> pki->ipi6_ifindex = 0;
> >>> ipv6_addr_copy(&pki->ipi6_addr,
> >>> - &rqstp->rq_daddr.addr6);
> >>> + &svc_daddr_in6(rqstp)->sin6_addr);
> >> I kind of suspect more is needed here to deal with ipi6_ifindex.
>
> Got it.
>
> >>
> >>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> >>> }
> >>> break;
> >>> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> >>> struct in_pktinfo *pki = CMSG_DATA(cmh);
> >>> if (cmh->cmsg_type != IP_PKTINFO)
> >>> return 0;
> >>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> >>> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> >>> return 1;
> >>> }
> >>>
> >>> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> >>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> >>> if (cmh->cmsg_type != IPV6_PKTINFO)
> >>> return 0;
> >>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> >>> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
> >> Likewise, is the incoming interface index ignored when constructing svc_daddr_in6(rqstp) ?
> >
> > Also, I can't seem to find where the address family field of svc_daddr() is being set.
> >
>
> Have complete it.
>
> thanks,
> Mi Jinlong
>
>
> From 84420475509ccc39ac8899f6f63140879a5278b6 Mon Sep 17 00:00:00 2001
> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> Date: Mon, 26 Aug 2011 13:22:39 +0800
> Subject: [PATCH v2] SUNRPC: Replace svc_addr_u by sockaddr_storage
>
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> besides address.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 25 ++-----------------------
> fs/nfsd/nfs4state.c | 16 +---------------
> include/linux/sunrpc/clnt.h | 3 ++-
> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> net/sunrpc/svc_xprt.c | 13 ++-----------
> net/sunrpc/svcsock.c | 23 +++++++++++++++++------
> 6 files changed, 45 insertions(+), 65 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..6f29836 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> struct hlist_node *pos;
> struct nlm_host *host = NULL;
> struct nsm_handle *nsm = NULL;
> - struct sockaddr_in sin = {
> - .sin_family = AF_INET,
> - };
> - struct sockaddr_in6 sin6 = {
> - .sin6_family = AF_INET6,
> - };
> - struct sockaddr *src_sap;
> - size_t src_len = rqstp->rq_addrlen;
> + struct sockaddr *src_sap = svc_daddr(rqstp);
> + size_t src_len = rqstp->rq_daddrlen;
> struct nlm_lookup_host_info ni = {
> .server = 1,
> .sap = svc_addr(rqstp),
> @@ -340,21 +334,6 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>
> mutex_lock(&nlm_host_mutex);
>
> - switch (ni.sap->sa_family) {
> - case AF_INET:
> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> - src_sap = (struct sockaddr *)&sin;
> - break;
> - case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> - src_sap = (struct sockaddr *)&sin6;
> - break;
> - default:
> - dprintk("lockd: %s failed; unrecognized address family\n",
> - __func__);
> - goto out;
> - }
> -
> if (time_after_eq(jiffies, next_gc))
> nlm_gc_hosts();
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..07a18a5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> return NULL;
> }
>
> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
> -{
> - switch (family) {
> - case AF_INET:
> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
> - return;
> - case AF_INET6:
> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> - return;
> - }
> -}
> -
> static void
> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
> {
> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index db7bcaf..1529a80 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -218,7 +218,8 @@ static inline bool __rpc_cmp_addr6(const struct sockaddr *sap1,
> {
> const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sap1;
> const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sap2;
> - return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr);
> + return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr)
> + && (sin1->sin6_scope_id == sin2->sin6_scope_id);
Chuck's earlier catches shamed me into looking at this more
critically. ;)
A question about the above... is this really correct? Consider the case
where we have a routable IPv6 address that's reachable via two
different interfaces. We'd almost certainly want to call those equal,
but this check would consider them to be different (assuming that the
scopeid is inherited from the incoming interface).
I'm still looking through the code to see if I can figure out what the
practical upshot of this would be, but might it be safer to limit the
scopeid check to just link-local addresses?
> }
>
> static inline bool __rpc_copy_addr6(struct sockaddr *dst,
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..3bc0dc9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> -union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> -};
> -
> /*
> * The context of a single thread, including the request currently being
> * processed.
> @@ -225,8 +220,12 @@ struct svc_rqst {
> struct list_head rq_list; /* idle list */
> struct list_head rq_all; /* all threads list */
> struct svc_xprt * rq_xprt; /* transport ptr */
> +
> struct sockaddr_storage rq_addr; /* peer address */
> size_t rq_addrlen;
> + struct sockaddr_storage rq_daddr; /* dest addr of request
> + * - reply from here */
> + size_t rq_daddrlen;
>
> struct svc_serv * rq_server; /* RPC service definition */
> struct svc_pool * rq_pool; /* thread pool */
> @@ -255,9 +254,6 @@ struct svc_rqst {
> unsigned short
> rq_secure : 1; /* secure port */
>
> - union svc_addr_u rq_daddr; /* dest addr of request
> - * - reply from here */
> -
> void * rq_argp; /* decoded arguments */
> void * rq_resp; /* xdr'd results */
> void * rq_auth_data; /* flavor-specific data */
> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_addr;
> }
>
> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr *) &rqst->rq_daddr;
> +}
> +
> /*
> * Check buffer bounds after decoding arguments
> */
> @@ -340,7 +351,8 @@ struct svc_deferred_req {
> struct svc_xprt *xprt;
> struct sockaddr_storage addr; /* where reply must go */
> size_t addrlen;
> - union svc_addr_u daddr; /* where reply must come from */
> + struct sockaddr_storage daddr; /* where reply must come from */
> + size_t daddrlen;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..d86bb67 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
> */
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> {
> - struct sockaddr *sin;
> -
> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
> rqstp->rq_addrlen = xprt->xpt_remotelen;
>
> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * Destination address in request is needed for binding the
> * source address in RPC replies/callbacks later.
> */
> - sin = (struct sockaddr *)&xprt->xpt_local;
> - switch (sin->sa_family) {
> - case AF_INET:
> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> - break;
> - case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> - break;
> - }
> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
> + rqstp->rq_daddrlen = xprt->xpt_locallen;
> }
> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..dfd686e 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -143,19 +143,20 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_level = SOL_IP;
> cmh->cmsg_type = IP_PKTINFO;
> pki->ipi_ifindex = 0;
> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
> + pki->ipi_spec_dst.s_addr =
> + svc_daddr_in(rqstp)->sin_addr.s_addr;
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
>
> case AF_INET6: {
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>
> cmh->cmsg_level = SOL_IPV6;
> cmh->cmsg_type = IPV6_PKTINFO;
> - pki->ipi6_ifindex = 0;
> - ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + pki->ipi6_ifindex = daddr->sin6_scope_id;
> + ipv6_addr_copy(&pki->ipi6_addr, &daddr->sin6_addr);
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -498,9 +499,13 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> struct cmsghdr *cmh)
> {
> struct in_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in *daddr = svc_daddr_in(rqstp);
> +
> if (cmh->cmsg_type != IP_PKTINFO)
> return 0;
> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> +
> + daddr->sin_family = AF_INET;
> + daddr->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> return 1;
> }
>
> @@ -511,9 +516,14 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct cmsghdr *cmh)
> {
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
> +
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> +
> + daddr->sin6_family = AF_INET6;
> + ipv6_addr_copy(&daddr->sin6_addr, &pki->ipi6_addr);
> + daddr->sin6_scope_id = pki->ipi6_ifindex;
> return 1;
> }
>
> @@ -614,6 +624,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> skb_free_datagram_locked(svsk->sk_sk, skb);
> return 0;
> }
> + rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
>
> if (skb_is_nonlinear(skb)) {
> /* we have to copy */
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-29 15:26 ` Jeff Layton
@ 2011-08-29 15:30 ` Chuck Lever
0 siblings, 0 replies; 24+ messages in thread
From: Chuck Lever @ 2011-08-29 15:30 UTC (permalink / raw)
To: Jeff Layton; +Cc: Mi Jinlong, J. Bruce Fields, NFS, Trond Myklebust
On Aug 29, 2011, at 11:26 AM, Jeff Layton wrote:
> On Fri, 26 Aug 2011 14:58:23 +0800
> Mi Jinlong <mijinlong@cn.fujitsu.com> wrote:
>
>> Thanks for you comment!
>>
>> Chuck Lever:
>>> On Aug 25, 2011, at 10:30 AM, Chuck Lever wrote:
>>>
>>>> This seems like a good direction to go in. More comments below.
>>>>
>>>> On Aug 24, 2011, at 1:58 AM, Mi Jinlong wrote:
>>>>
>>>>> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>>>> Date: Mon, 24 Aug 2011 13:22:39 +0800
>>>>> Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage
>>>>>
>>>>> For IPv6 local address, lockd can not callback to client for
>>>>> missing scope id when binding address at inet6_bind:
>>>>>
>>>>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>>>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>>>>> 326 addr->sin6_scope_id) {
>>>>> 327 /* Override any existing binding, if another one
>>>>> 328 * is supplied by user.
>>>>> 329 */
>>>>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>>>>> 331 }
>>>>> 332
>>>>> 333 /* Binding to link-local address requires an interface */
>>>>> 334 if (!sk->sk_bound_dev_if) {
>>>>> 335 err = -EINVAL;
>>>>> 336 goto out_unlock;
>>>>> 337 }
>>>>>
>>>>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
>>>>> besides address.
>>>>>
>>>>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>>>> ---
>>>>> fs/lockd/host.c | 14 ++------------
>>>>> fs/nfsd/nfs4state.c | 16 +---------------
>>>>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
>>>>> net/sunrpc/svc_xprt.c | 13 ++-----------
>>>>> net/sunrpc/svcsock.c | 9 +++++----
>>>>> 5 files changed, 31 insertions(+), 51 deletions(-)
>>>>>
>>>>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>>>>> index b7c99bf..b23db05 100644
>>>>> --- a/fs/lockd/host.c
>>>>> +++ b/fs/lockd/host.c
>>>>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>>>> struct hlist_node *pos;
>>>>> struct nlm_host *host = NULL;
>>>>> struct nsm_handle *nsm = NULL;
>>>>> - struct sockaddr_in sin = {
>>>>> - .sin_family = AF_INET,
>>>>> - };
>>>>> - struct sockaddr_in6 sin6 = {
>>>>> - .sin6_family = AF_INET6,
>>>>> - };
>>>>> struct sockaddr *src_sap;
>>>>> - size_t src_len = rqstp->rq_addrlen;
>>>>> + size_t src_len = rqstp->rq_daddrlen;
>>>>> struct nlm_lookup_host_info ni = {
>>>>> .server = 1,
>>>>> .sap = svc_addr(rqstp),
>>>>> @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>>>>
>>>>> switch (ni.sap->sa_family) {
>>>>> case AF_INET:
>>>>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
>>>>> - src_sap = (struct sockaddr *)&sin;
>>>>> - break;
>>>>> case AF_INET6:
>>>>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
>>>>> - src_sap = (struct sockaddr *)&sin6;
>>>>> + src_sap = svc_daddr(rqstp);
>>>>> break;
>>>>> default:
>>>>> dprintk("lockd: %s failed; unrecognized address family\n",
>>>> With the destination address promoted to a full sockaddr, we probably do not need this switch any more.
>>
>> Got it.
>>
>>>>
>>>> I notice that the IPv6 fork of rpc_cmp_addr() does not consider the scope IDs of the passed-in addresses. Perhaps it should?
>>
>> Have add scope ID compare at __rpc_cmp_addr6 as following patch.
>>
>>>>
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index 3787ec1..07a18a5 100644
>>>>> --- a/fs/nfsd/nfs4state.c
>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>>>>> return NULL;
>>>>> }
>> ... snip ...
>>>>> break;
>>>>> @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>>>>> cmh->cmsg_type = IPV6_PKTINFO;
>>>>> pki->ipi6_ifindex = 0;
>>>>> ipv6_addr_copy(&pki->ipi6_addr,
>>>>> - &rqstp->rq_daddr.addr6);
>>>>> + &svc_daddr_in6(rqstp)->sin6_addr);
>>>> I kind of suspect more is needed here to deal with ipi6_ifindex.
>>
>> Got it.
>>
>>>>
>>>>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>>>>> }
>>>>> break;
>>>>> @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
>>>>> struct in_pktinfo *pki = CMSG_DATA(cmh);
>>>>> if (cmh->cmsg_type != IP_PKTINFO)
>>>>> return 0;
>>>>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>>>>> + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>>>>> return 1;
>>>>> }
>>>>>
>>>>> @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
>>>>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>>>>> if (cmh->cmsg_type != IPV6_PKTINFO)
>>>>> return 0;
>>>>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>>>>> + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr);
>>>> Likewise, is the incoming interface index ignored when constructing svc_daddr_in6(rqstp) ?
>>>
>>> Also, I can't seem to find where the address family field of svc_daddr() is being set.
>>>
>>
>> Have complete it.
>>
>> thanks,
>> Mi Jinlong
>>
>>
>> From 84420475509ccc39ac8899f6f63140879a5278b6 Mon Sep 17 00:00:00 2001
>> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> Date: Mon, 26 Aug 2011 13:22:39 +0800
>> Subject: [PATCH v2] SUNRPC: Replace svc_addr_u by sockaddr_storage
>>
>> For IPv6 local address, lockd can not callback to client for
>> missing scope id when binding address at inet6_bind:
>>
>> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
>> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
>> 326 addr->sin6_scope_id) {
>> 327 /* Override any existing binding, if another one
>> 328 * is supplied by user.
>> 329 */
>> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
>> 331 }
>> 332
>> 333 /* Binding to link-local address requires an interface */
>> 334 if (!sk->sk_bound_dev_if) {
>> 335 err = -EINVAL;
>> 336 goto out_unlock;
>> 337 }
>>
>> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
>> besides address.
>>
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>> fs/lockd/host.c | 25 ++-----------------------
>> fs/nfsd/nfs4state.c | 16 +---------------
>> include/linux/sunrpc/clnt.h | 3 ++-
>> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
>> net/sunrpc/svc_xprt.c | 13 ++-----------
>> net/sunrpc/svcsock.c | 23 +++++++++++++++++------
>> 6 files changed, 45 insertions(+), 65 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index b7c99bf..6f29836 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>> struct hlist_node *pos;
>> struct nlm_host *host = NULL;
>> struct nsm_handle *nsm = NULL;
>> - struct sockaddr_in sin = {
>> - .sin_family = AF_INET,
>> - };
>> - struct sockaddr_in6 sin6 = {
>> - .sin6_family = AF_INET6,
>> - };
>> - struct sockaddr *src_sap;
>> - size_t src_len = rqstp->rq_addrlen;
>> + struct sockaddr *src_sap = svc_daddr(rqstp);
>> + size_t src_len = rqstp->rq_daddrlen;
>> struct nlm_lookup_host_info ni = {
>> .server = 1,
>> .sap = svc_addr(rqstp),
>> @@ -340,21 +334,6 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>>
>> mutex_lock(&nlm_host_mutex);
>>
>> - switch (ni.sap->sa_family) {
>> - case AF_INET:
>> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
>> - src_sap = (struct sockaddr *)&sin;
>> - break;
>> - case AF_INET6:
>> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
>> - src_sap = (struct sockaddr *)&sin6;
>> - break;
>> - default:
>> - dprintk("lockd: %s failed; unrecognized address family\n",
>> - __func__);
>> - goto out;
>> - }
>> -
>> if (time_after_eq(jiffies, next_gc))
>> nlm_gc_hosts();
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 3787ec1..07a18a5 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>> return NULL;
>> }
>>
>> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
>> -{
>> - switch (family) {
>> - case AF_INET:
>> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
>> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
>> - return;
>> - case AF_INET6:
>> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
>> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
>> - return;
>> - }
>> -}
>> -
>> static void
>> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
>> {
>> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>>
>> conn->cb_prog = se->se_callback_prog;
>> conn->cb_ident = se->se_callback_ident;
>> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
>> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
>> return;
>> out_err:
>> conn->cb_addr.ss_family = AF_UNSPEC;
>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>> index db7bcaf..1529a80 100644
>> --- a/include/linux/sunrpc/clnt.h
>> +++ b/include/linux/sunrpc/clnt.h
>> @@ -218,7 +218,8 @@ static inline bool __rpc_cmp_addr6(const struct sockaddr *sap1,
>> {
>> const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sap1;
>> const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sap2;
>> - return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr);
>> + return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr)
>> + && (sin1->sin6_scope_id == sin2->sin6_scope_id);
>
>
> Chuck's earlier catches shamed me into looking at this more
> critically. ;)
>
> A question about the above... is this really correct? Consider the case
> where we have a routable IPv6 address that's reachable via two
> different interfaces. We'd almost certainly want to call those equal,
> but this check would consider them to be different (assuming that the
> scopeid is inherited from the incoming interface).
>
> I'm still looking through the code to see if I can figure out what the
> practical upshot of this would be, but might it be safer to limit the
> scopeid check to just link-local addresses?
I think that's correct. Scope ID should be compared only on link-local addresses.
And maybe the rpc_cmp_addr() changes could be split into a separate patch.
>
>> }
>>
>> static inline bool __rpc_copy_addr6(struct sockaddr *dst,
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 223588a..3bc0dc9 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
>> iov->iov_len += sizeof(__be32);
>> }
>>
>> -union svc_addr_u {
>> - struct in_addr addr;
>> - struct in6_addr addr6;
>> -};
>> -
>> /*
>> * The context of a single thread, including the request currently being
>> * processed.
>> @@ -225,8 +220,12 @@ struct svc_rqst {
>> struct list_head rq_list; /* idle list */
>> struct list_head rq_all; /* all threads list */
>> struct svc_xprt * rq_xprt; /* transport ptr */
>> +
>> struct sockaddr_storage rq_addr; /* peer address */
>> size_t rq_addrlen;
>> + struct sockaddr_storage rq_daddr; /* dest addr of request
>> + * - reply from here */
>> + size_t rq_daddrlen;
>>
>> struct svc_serv * rq_server; /* RPC service definition */
>> struct svc_pool * rq_pool; /* thread pool */
>> @@ -255,9 +254,6 @@ struct svc_rqst {
>> unsigned short
>> rq_secure : 1; /* secure port */
>>
>> - union svc_addr_u rq_daddr; /* dest addr of request
>> - * - reply from here */
>> -
>> void * rq_argp; /* decoded arguments */
>> void * rq_resp; /* xdr'd results */
>> void * rq_auth_data; /* flavor-specific data */
>> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
>> return (struct sockaddr *) &rqst->rq_addr;
>> }
>>
>> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
>> +}
>> +
>> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
>> +{
>> + return (struct sockaddr *) &rqst->rq_daddr;
>> +}
>> +
>> /*
>> * Check buffer bounds after decoding arguments
>> */
>> @@ -340,7 +351,8 @@ struct svc_deferred_req {
>> struct svc_xprt *xprt;
>> struct sockaddr_storage addr; /* where reply must go */
>> size_t addrlen;
>> - union svc_addr_u daddr; /* where reply must come from */
>> + struct sockaddr_storage daddr; /* where reply must come from */
>> + size_t daddrlen;
>> struct cache_deferred_req handle;
>> size_t xprt_hlen;
>> int argslen;
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index bd31208..d86bb67 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
>> */
>> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> {
>> - struct sockaddr *sin;
>> -
>> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
>> rqstp->rq_addrlen = xprt->xpt_remotelen;
>>
>> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
>> * Destination address in request is needed for binding the
>> * source address in RPC replies/callbacks later.
>> */
>> - sin = (struct sockaddr *)&xprt->xpt_local;
>> - switch (sin->sa_family) {
>> - case AF_INET:
>> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
>> - break;
>> - case AF_INET6:
>> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
>> - break;
>> - }
>> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
>> + rqstp->rq_daddrlen = xprt->xpt_locallen;
>> }
>> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 767d494..dfd686e 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -143,19 +143,20 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
>> cmh->cmsg_level = SOL_IP;
>> cmh->cmsg_type = IP_PKTINFO;
>> pki->ipi_ifindex = 0;
>> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
>> + pki->ipi_spec_dst.s_addr =
>> + svc_daddr_in(rqstp)->sin_addr.s_addr;
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>>
>> case AF_INET6: {
>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>>
>> cmh->cmsg_level = SOL_IPV6;
>> cmh->cmsg_type = IPV6_PKTINFO;
>> - pki->ipi6_ifindex = 0;
>> - ipv6_addr_copy(&pki->ipi6_addr,
>> - &rqstp->rq_daddr.addr6);
>> + pki->ipi6_ifindex = daddr->sin6_scope_id;
>> + ipv6_addr_copy(&pki->ipi6_addr, &daddr->sin6_addr);
>> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
>> }
>> break;
>> @@ -498,9 +499,13 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
>> struct cmsghdr *cmh)
>> {
>> struct in_pktinfo *pki = CMSG_DATA(cmh);
>> + struct sockaddr_in *daddr = svc_daddr_in(rqstp);
>> +
>> if (cmh->cmsg_type != IP_PKTINFO)
>> return 0;
>> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>> +
>> + daddr->sin_family = AF_INET;
>> + daddr->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>> return 1;
>> }
>>
>> @@ -511,9 +516,14 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
>> struct cmsghdr *cmh)
>> {
>> struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>> +
>> if (cmh->cmsg_type != IPV6_PKTINFO)
>> return 0;
>> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>> +
>> + daddr->sin6_family = AF_INET6;
>> + ipv6_addr_copy(&daddr->sin6_addr, &pki->ipi6_addr);
>> + daddr->sin6_scope_id = pki->ipi6_ifindex;
>> return 1;
>> }
>>
>> @@ -614,6 +624,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>> skb_free_datagram_locked(svsk->sk_sk, skb);
>> return 0;
>> }
>> + rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
>>
>> if (skb_is_nonlinear(skb)) {
>> /* we have to copy */
>
>
> --
> Jeff Layton <jlayton@redhat.com>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address
2011-08-26 6:58 ` Mi Jinlong
2011-08-26 22:43 ` J. Bruce Fields
2011-08-29 15:26 ` Jeff Layton
@ 2011-08-29 17:09 ` Steve Dickson
2 siblings, 0 replies; 24+ messages in thread
From: Steve Dickson @ 2011-08-29 17:09 UTC (permalink / raw)
To: Mi Jinlong; +Cc: Chuck Lever, J. Bruce Fields, NFS, Trond Myklebust
Hey Mi,
If this is the final version of the patch, could you
please post it new thread? Actually please post all new
versions of patch in new threads, since its just
really difficulty to pull bits and pieces out of
a huge emails like this one...
Plus this patch do not apply...
tia,
steved.
On 08/26/2011 02:58 AM, Mi Jinlong wrote:
> From 84420475509ccc39ac8899f6f63140879a5278b6 Mon Sep 17 00:00:00 2001
> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> Date: Mon, 26 Aug 2011 13:22:39 +0800
> Subject: [PATCH v2] SUNRPC: Replace svc_addr_u by sockaddr_storage
>
> For IPv6 local address, lockd can not callback to client for
> missing scope id when binding address at inet6_bind:
>
> 324 if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 325 if (addr_len >= sizeof(struct sockaddr_in6) &&
> 326 addr->sin6_scope_id) {
> 327 /* Override any existing binding, if another one
> 328 * is supplied by user.
> 329 */
> 330 sk->sk_bound_dev_if = addr->sin6_scope_id;
> 331 }
> 332
> 333 /* Binding to link-local address requires an interface */
> 334 if (!sk->sk_bound_dev_if) {
> 335 err = -EINVAL;
> 336 goto out_unlock;
> 337 }
>
> Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info
> besides address.
>
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
> fs/lockd/host.c | 25 ++-----------------------
> fs/nfsd/nfs4state.c | 16 +---------------
> include/linux/sunrpc/clnt.h | 3 ++-
> include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++---------
> net/sunrpc/svc_xprt.c | 13 ++-----------
> net/sunrpc/svcsock.c | 23 +++++++++++++++++------
> 6 files changed, 45 insertions(+), 65 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index b7c99bf..6f29836 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
> struct hlist_node *pos;
> struct nlm_host *host = NULL;
> struct nsm_handle *nsm = NULL;
> - struct sockaddr_in sin = {
> - .sin_family = AF_INET,
> - };
> - struct sockaddr_in6 sin6 = {
> - .sin6_family = AF_INET6,
> - };
> - struct sockaddr *src_sap;
> - size_t src_len = rqstp->rq_addrlen;
> + struct sockaddr *src_sap = svc_daddr(rqstp);
> + size_t src_len = rqstp->rq_daddrlen;
> struct nlm_lookup_host_info ni = {
> .server = 1,
> .sap = svc_addr(rqstp),
> @@ -340,21 +334,6 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>
> mutex_lock(&nlm_host_mutex);
>
> - switch (ni.sap->sa_family) {
> - case AF_INET:
> - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
> - src_sap = (struct sockaddr *)&sin;
> - break;
> - case AF_INET6:
> - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6);
> - src_sap = (struct sockaddr *)&sin6;
> - break;
> - default:
> - dprintk("lockd: %s failed; unrecognized address family\n",
> - __func__);
> - goto out;
> - }
> -
> if (time_after_eq(jiffies, next_gc))
> nlm_gc_hosts();
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3787ec1..07a18a5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1173,20 +1173,6 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
> return NULL;
> }
>
> -static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr)
> -{
> - switch (family) {
> - case AF_INET:
> - ((struct sockaddr_in *)sa)->sin_family = AF_INET;
> - ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr;
> - return;
> - case AF_INET6:
> - ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6;
> - ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6;
> - return;
> - }
> -}
> -
> static void
> gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp)
> {
> @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r
>
> conn->cb_prog = se->se_callback_prog;
> conn->cb_ident = se->se_callback_ident;
> - rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr);
> + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen);
> return;
> out_err:
> conn->cb_addr.ss_family = AF_UNSPEC;
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index db7bcaf..1529a80 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -218,7 +218,8 @@ static inline bool __rpc_cmp_addr6(const struct sockaddr *sap1,
> {
> const struct sockaddr_in6 *sin1 = (const struct sockaddr_in6 *)sap1;
> const struct sockaddr_in6 *sin2 = (const struct sockaddr_in6 *)sap2;
> - return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr);
> + return ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr)
> + && (sin1->sin6_scope_id == sin2->sin6_scope_id);
> }
>
> static inline bool __rpc_copy_addr6(struct sockaddr *dst,
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 223588a..3bc0dc9 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val)
> iov->iov_len += sizeof(__be32);
> }
>
> -union svc_addr_u {
> - struct in_addr addr;
> - struct in6_addr addr6;
> -};
> -
> /*
> * The context of a single thread, including the request currently being
> * processed.
> @@ -225,8 +220,12 @@ struct svc_rqst {
> struct list_head rq_list; /* idle list */
> struct list_head rq_all; /* all threads list */
> struct svc_xprt * rq_xprt; /* transport ptr */
> +
> struct sockaddr_storage rq_addr; /* peer address */
> size_t rq_addrlen;
> + struct sockaddr_storage rq_daddr; /* dest addr of request
> + * - reply from here */
> + size_t rq_daddrlen;
>
> struct svc_serv * rq_server; /* RPC service definition */
> struct svc_pool * rq_pool; /* thread pool */
> @@ -255,9 +254,6 @@ struct svc_rqst {
> unsigned short
> rq_secure : 1; /* secure port */
>
> - union svc_addr_u rq_daddr; /* dest addr of request
> - * - reply from here */
> -
> void * rq_argp; /* decoded arguments */
> void * rq_resp; /* xdr'd results */
> void * rq_auth_data; /* flavor-specific data */
> @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_addr;
> }
>
> +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr_in6 *) &rqst->rq_daddr;
> +}
> +
> +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> +{
> + return (struct sockaddr *) &rqst->rq_daddr;
> +}
> +
> /*
> * Check buffer bounds after decoding arguments
> */
> @@ -340,7 +351,8 @@ struct svc_deferred_req {
> struct svc_xprt *xprt;
> struct sockaddr_storage addr; /* where reply must go */
> size_t addrlen;
> - union svc_addr_u daddr; /* where reply must come from */
> + struct sockaddr_storage daddr; /* where reply must come from */
> + size_t daddrlen;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index bd31208..d86bb67 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
> */
> void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> {
> - struct sockaddr *sin;
> -
> memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen);
> rqstp->rq_addrlen = xprt->xpt_remotelen;
>
> @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * Destination address in request is needed for binding the
> * source address in RPC replies/callbacks later.
> */
> - sin = (struct sockaddr *)&xprt->xpt_local;
> - switch (sin->sa_family) {
> - case AF_INET:
> - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr;
> - break;
> - case AF_INET6:
> - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr;
> - break;
> - }
> + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen);
> + rqstp->rq_daddrlen = xprt->xpt_locallen;
> }
> EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs);
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..dfd686e 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -143,19 +143,20 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh)
> cmh->cmsg_level = SOL_IP;
> cmh->cmsg_type = IP_PKTINFO;
> pki->ipi_ifindex = 0;
> - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr;
> + pki->ipi_spec_dst.s_addr =
> + svc_daddr_in(rqstp)->sin_addr.s_addr;
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
>
> case AF_INET6: {
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
>
> cmh->cmsg_level = SOL_IPV6;
> cmh->cmsg_type = IPV6_PKTINFO;
> - pki->ipi6_ifindex = 0;
> - ipv6_addr_copy(&pki->ipi6_addr,
> - &rqstp->rq_daddr.addr6);
> + pki->ipi6_ifindex = daddr->sin6_scope_id;
> + ipv6_addr_copy(&pki->ipi6_addr, &daddr->sin6_addr);
> cmh->cmsg_len = CMSG_LEN(sizeof(*pki));
> }
> break;
> @@ -498,9 +499,13 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp,
> struct cmsghdr *cmh)
> {
> struct in_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in *daddr = svc_daddr_in(rqstp);
> +
> if (cmh->cmsg_type != IP_PKTINFO)
> return 0;
> - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> +
> + daddr->sin_family = AF_INET;
> + daddr->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
> return 1;
> }
>
> @@ -511,9 +516,14 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp,
> struct cmsghdr *cmh)
> {
> struct in6_pktinfo *pki = CMSG_DATA(cmh);
> + struct sockaddr_in6 *daddr = svc_daddr_in6(rqstp);
> +
> if (cmh->cmsg_type != IPV6_PKTINFO)
> return 0;
> - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> +
> + daddr->sin6_family = AF_INET6;
> + ipv6_addr_copy(&daddr->sin6_addr, &pki->ipi6_addr);
> + daddr->sin6_scope_id = pki->ipi6_ifindex;
> return 1;
> }
>
> @@ -614,6 +624,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> skb_free_datagram_locked(svsk->sk_sk, skb);
> return 0;
> }
> + rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
>
> if (skb_is_nonlinear(skb)) {
> /* we have to copy */
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2011-08-29 17:09 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-20 10:24 [PATCH] sunrpc: Add scope id to svc_addr_u for IPv6 LINKLOCAL address Mi Jinlong
2011-08-22 19:23 ` J. Bruce Fields
2011-08-22 19:39 ` Jeff Layton
2011-08-22 19:44 ` Chuck Lever
2011-08-22 21:08 ` J. Bruce Fields
2011-08-23 14:43 ` Jeff Layton
2011-08-24 8:00 ` Mi Jinlong
2011-08-24 15:15 ` Chuck Lever
2011-08-22 19:26 ` J. Bruce Fields
2011-08-22 19:32 ` Chuck Lever
2011-08-22 19:44 ` J. Bruce Fields
2011-08-23 9:40 ` Mi Jinlong
2011-08-24 5:58 ` Mi Jinlong
2011-08-24 18:39 ` J. Bruce Fields
2011-08-25 0:02 ` Mi Jinlong
2011-08-25 9:58 ` Jeff Layton
2011-08-25 14:30 ` Chuck Lever
2011-08-25 14:37 ` Chuck Lever
2011-08-26 6:58 ` Mi Jinlong
2011-08-26 22:43 ` J. Bruce Fields
2011-08-27 22:03 ` Chuck Lever
2011-08-29 15:26 ` Jeff Layton
2011-08-29 15:30 ` Chuck Lever
2011-08-29 17:09 ` Steve Dickson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.