[RFC,PATCH] svc: REPOST - Fix the UDP address logic

[RFC,PATCH] svc: REPOST - Fix the UDP address logic

[Tom Tucker]

This version of the patch sets xpt_remotelen based on the address
family returned in the sockaddr. If the address family is not
AF_INET or AF_INET6, the svc_udp_recvfrom function will
return -ENOTSUPP.
--

When the address information was moved to the svc_xprt structure, a bug
was introduced to UDP that caused an incorrect address to be used
when responding to RPC on the UDP transport. This was the result of
failing to completely implement the generic address logic for the UDP 
transport.

Thanks to Greg for pointing this out...

Since I confused myself, it's probably a good idea to describe how 
this is supposed to work. The transport is responsible for setting 
the xpt_local and xpt_remote addresses in the svc_xprt structure as
part of xpo_recvfrom processing. This cannot be done in a generic way 
and in fact varies between TCP, UDP and RDMA. A set of xpo_ functions
(e.g. getlocalname, getremotename) could have been added but this would
have resulted in additional caching and copying of the addresses around. 

The generic svc_recv code copies the addresses from the svc_xprt 
structure into the rqstp structure as part of svc_recv processing.  

The xpt_local address should also be set on listening endpoints, for
tcp/rdma this is done as part of endpoint creation and for new 
connections in xpo_accept processing. 

This patch was tested with Connectathon over V3 on UDP.

Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
---

 net/sunrpc/svcsock.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index e1a27ee..8924cad 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -140,6 +140,18 @@ static char *__svc_print_addr(struct soc
 	return buf;
 }
 
+static size_t svc_sockaddr_len(struct sockaddr *addr)
+{
+	switch (addr->sa_family) {
+	case AF_INET:
+		return sizeof(struct sockaddr_in);
+	case AF_INET6:
+		return sizeof(struct sockaddr_in6);
+	}
+	dprintk("svc: unrecognized address family %d\n", addr->sa_family);
+	return -ENOTSUPP;
+}
+
 /**
  * svc_print_addr - Format rq_addr field for printing
  * @rqstp: svc_rqst struct containing address to print
@@ -473,17 +485,21 @@ svc_write_space(struct sock *sk)
 static inline void svc_udp_get_dest_address(struct svc_rqst *rqstp,
 					    struct cmsghdr *cmh)
 {
-	struct svc_sock *svsk =
-		container_of(rqstp->rq_xprt, struct svc_sock, sk_xprt);
+	struct svc_xprt *xprt = rqstp->rq_xprt;
+	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
 	switch (svsk->sk_sk->sk_family) {
 	case AF_INET: {
 		struct in_pktinfo *pki = CMSG_DATA(cmh);
-		rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
+		struct sockaddr_in *sin =
+			(struct sockaddr_in *)&xprt->xpt_local;
+		sin->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
 		break;
 		}
 	case AF_INET6: {
 		struct in6_pktinfo *pki = CMSG_DATA(cmh);
-		ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
+		struct sockaddr_in6 *sin6 =
+			(struct sockaddr_in6 *)&xprt->xpt_local;
+		ipv6_addr_copy(&sin6->sin6_addr, &pki->ipi6_addr);
 		break;
 		}
 	}
@@ -506,7 +522,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 	struct cmsghdr *cmh = &buffer.hdr;
 	int		err, len;
 	struct msghdr msg = {
-		.msg_name = svc_addr(rqstp),
+		.msg_name = &svsk->sk_xprt.xpt_remote,
 		.msg_control = cmh,
 		.msg_controllen = sizeof(buffer),
 		.msg_flags = MSG_DONTWAIT,
@@ -541,7 +557,10 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 		svc_xprt_received(&svsk->sk_xprt);
 		return -EAGAIN;
 	}
-	rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
+	len = svc_sockaddr_len((struct sockaddr *)&svsk->sk_xprt.xpt_remote);
+	if (len < 0)
+		return len;
+	svsk->sk_xprt.xpt_remotelen = len;
 	if (skb->tstamp.tv64 == 0) {
 		skb->tstamp = ktime_get_real();
 		/* Don't enable netstamp, sunrpc doesn't

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH] svc: REPOST - Fix the UDP address logic

[Chuck Lever]

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

Tom Tucker wrote:
> This version of the patch sets xpt_remotelen based on the address
> family returned in the sockaddr. If the address family is not
> AF_INET or AF_INET6, the svc_udp_recvfrom function will
> return -ENOTSUPP.
> --
> 
> When the address information was moved to the svc_xprt structure, a bug
> was introduced to UDP that caused an incorrect address to be used
> when responding to RPC on the UDP transport. This was the result of
> failing to completely implement the generic address logic for the UDP 
> transport.
> 
> Thanks to Greg for pointing this out...
> 
> Since I confused myself, it's probably a good idea to describe how 
> this is supposed to work. The transport is responsible for setting 
> the xpt_local and xpt_remote addresses in the svc_xprt structure as
> part of xpo_recvfrom processing. This cannot be done in a generic way 
> and in fact varies between TCP, UDP and RDMA. A set of xpo_ functions
> (e.g. getlocalname, getremotename) could have been added but this would
> have resulted in additional caching and copying of the addresses around. 
> 
> The generic svc_recv code copies the addresses from the svc_xprt 
> structure into the rqstp structure as part of svc_recv processing.  
> 
> The xpt_local address should also be set on listening endpoints, for
> tcp/rdma this is done as part of endpoint creation and for new 
> connections in xpo_accept processing. 
> 
> This patch was tested with Connectathon over V3 on UDP.
> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> ---
> 
>  net/sunrpc/svcsock.c |   31 +++++++++++++++++++++++++------
>  1 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index e1a27ee..8924cad 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -140,6 +140,18 @@ static char *__svc_print_addr(struct soc
>  	return buf;
>  }
>  
> +static size_t svc_sockaddr_len(struct sockaddr *addr)
> +{
> +	switch (addr->sa_family) {
> +	case AF_INET:
> +		return sizeof(struct sockaddr_in);
> +	case AF_INET6:
> +		return sizeof(struct sockaddr_in6);
> +	}
> +	dprintk("svc: unrecognized address family %d\n", addr->sa_family);
> +	return -ENOTSUPP;
> +}
> +

Nit: sa_family is probably an unsigned short, so use "%u" instead of
"%d".  Also I'm using -EAFNOSUPPORT in similar error cases, but I may 
have missed a stated preference.

I'm working blind here (not having pulled down Bruce's or your summary
branches).  I suspect you may need the result of svc_sockaddr_len() in
other parts of your code (perhaps in the RDMA transport implementation).

Based on your patch and comments, I assume that there are some missing
changes (from me, perhaps) that leave updating rq_addrlen with a
sizeof() instead of a real address size in several other places besides
the UDP receive path.  It would be cleaner to fix all of those in one or
more separate patches (placed before your "Fix the UDP address logic"
patch).

(There is a documented preference for this kind of patch atomizing and
ordering which you can probably find in Documentation/SubmittingPatches
or something like that).

>  /**
>   * svc_print_addr - Format rq_addr field for printing
>   * @rqstp: svc_rqst struct containing address to print
> @@ -473,17 +485,21 @@ svc_write_space(struct sock *sk)
>  static inline void svc_udp_get_dest_address(struct svc_rqst *rqstp,
>  					    struct cmsghdr *cmh)
>  {
> -	struct svc_sock *svsk =
> -		container_of(rqstp->rq_xprt, struct svc_sock, sk_xprt);
> +	struct svc_xprt *xprt = rqstp->rq_xprt;
> +	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
>  	switch (svsk->sk_sk->sk_family) {
>  	case AF_INET: {
>  		struct in_pktinfo *pki = CMSG_DATA(cmh);
> -		rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
> +		struct sockaddr_in *sin =
> +			(struct sockaddr_in *)&xprt->xpt_local;
> +		sin->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>  		break;
>  		}
>  	case AF_INET6: {
>  		struct in6_pktinfo *pki = CMSG_DATA(cmh);
> -		ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
> +		struct sockaddr_in6 *sin6 =
> +			(struct sockaddr_in6 *)&xprt->xpt_local;
> +		ipv6_addr_copy(&sin6->sin6_addr, &pki->ipi6_addr);
>  		break;
>  		}
>  	}

Neil asked me to add macros for casting sockaddr_storage fields in the 
svc_rqst structure (see svc_addr() and svc_addr_in()).  You might 
consider adding similar macros for cleanly extracting address 
information from svc_xprt.

> @@ -506,7 +522,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
>  	struct cmsghdr *cmh = &buffer.hdr;
>  	int		err, len;
>  	struct msghdr msg = {
> -		.msg_name = svc_addr(rqstp),
> +		.msg_name = &svsk->sk_xprt.xpt_remote,
>  		.msg_control = cmh,
>  		.msg_controllen = sizeof(buffer),
>  		.msg_flags = MSG_DONTWAIT,
> @@ -541,7 +557,10 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
>  		svc_xprt_received(&svsk->sk_xprt);
>  		return -EAGAIN;
>  	}
> -	rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
> +	len = svc_sockaddr_len((struct sockaddr *)&svsk->sk_xprt.xpt_remote);
> +	if (len < 0)
> +		return len;
> +	svsk->sk_xprt.xpt_remotelen = len;
>  	if (skb->tstamp.tv64 == 0) {
>  		skb->tstamp = ktime_get_real();
>  		/* Don't enable netstamp, sunrpc doesn't

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

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
email;internet:chuck dot lever at nospam oracle dot com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH] svc: REPOST - Fix the UDP address logic

[Tom Tucker]

Chuck Lever wrote:
> Tom Tucker wrote:
>> This version of the patch sets xpt_remotelen based on the address
>> family returned in the sockaddr. If the address family is not
>> AF_INET or AF_INET6, the svc_udp_recvfrom function will
>> return -ENOTSUPP.
>> -- 
>>
>> When the address information was moved to the svc_xprt structure, a bug
>> was introduced to UDP that caused an incorrect address to be used
>> when responding to RPC on the UDP transport. This was the result of
>> failing to completely implement the generic address logic for the UDP 
>> transport.
>>
>> Thanks to Greg for pointing this out...
>>
>> Since I confused myself, it's probably a good idea to describe how 
>> this is supposed to work. The transport is responsible for setting 
>> the xpt_local and xpt_remote addresses in the svc_xprt structure as
>> part of xpo_recvfrom processing. This cannot be done in a generic way 
>> and in fact varies between TCP, UDP and RDMA. A set of xpo_ functions
>> (e.g. getlocalname, getremotename) could have been added but this would
>> have resulted in additional caching and copying of the addresses around.
>> The generic svc_recv code copies the addresses from the svc_xprt 
>> structure into the rqstp structure as part of svc_recv processing. 
>> The xpt_local address should also be set on listening endpoints, for
>> tcp/rdma this is done as part of endpoint creation and for new 
>> connections in xpo_accept processing.
>> This patch was tested with Connectathon over V3 on UDP.
>>
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> ---
>>
>>  net/sunrpc/svcsock.c |   31 +++++++++++++++++++++++++------
>>  1 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index e1a27ee..8924cad 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -140,6 +140,18 @@ static char *__svc_print_addr(struct soc
>>      return buf;
>>  }
>>  
>> +static size_t svc_sockaddr_len(struct sockaddr *addr)
>> +{
>> +    switch (addr->sa_family) {
>> +    case AF_INET:
>> +        return sizeof(struct sockaddr_in);
>> +    case AF_INET6:
>> +        return sizeof(struct sockaddr_in6);
>> +    }
>> +    dprintk("svc: unrecognized address family %d\n", addr->sa_family);
>> +    return -ENOTSUPP;
>> +}
>> +
>
> Nit: sa_family is probably an unsigned short, so use "%u" instead of
> "%d".  Also I'm using -EAFNOSUPPORT in similar error cases, but I may 
> have missed a stated preference.
No, I think you're right :-\  Erf...one more time!
>
> I'm working blind here (not having pulled down Bruce's or your summary
> branches).  I suspect you may need the result of svc_sockaddr_len() in
> other parts of your code (perhaps in the RDMA transport implementation).
>
I thought about that too and did a somewhat cursory search. I didn't see 
anything obvious. The TCP side uses kernel_getpeername and it sets the 
length.
> Based on your patch and comments, I assume that there are some missing
> changes (from me, perhaps) that leave updating rq_addrlen with a
> sizeof() instead of a real address size in several other places besides
> the UDP receive path.  It would be cleaner to fix all of those in one or
> more separate patches (placed before your "Fix the UDP address logic"
> patch).
Ok, I'll do this when I do the perfect hindsight roll up.
>
> (There is a documented preference for this kind of patch atomizing and
> ordering which you can probably find in Documentation/SubmittingPatches
> or something like that).
>
Thanks, I'll peruse it.
>>  /**
>>   * svc_print_addr - Format rq_addr field for printing
>>   * @rqstp: svc_rqst struct containing address to print
>> @@ -473,17 +485,21 @@ svc_write_space(struct sock *sk)
>>  static inline void svc_udp_get_dest_address(struct svc_rqst *rqstp,
>>                          struct cmsghdr *cmh)
>>  {
>> -    struct svc_sock *svsk =
>> -        container_of(rqstp->rq_xprt, struct svc_sock, sk_xprt);
>> +    struct svc_xprt *xprt = rqstp->rq_xprt;
>> +    struct svc_sock *svsk = container_of(xprt, struct svc_sock, 
>> sk_xprt);
>>      switch (svsk->sk_sk->sk_family) {
>>      case AF_INET: {
>>          struct in_pktinfo *pki = CMSG_DATA(cmh);
>> -        rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr;
>> +        struct sockaddr_in *sin =
>> +            (struct sockaddr_in *)&xprt->xpt_local;
>> +        sin->sin_addr.s_addr = pki->ipi_spec_dst.s_addr;
>>          break;
>>          }
>>      case AF_INET6: {
>>          struct in6_pktinfo *pki = CMSG_DATA(cmh);
>> -        ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr);
>> +        struct sockaddr_in6 *sin6 =
>> +            (struct sockaddr_in6 *)&xprt->xpt_local;
>> +        ipv6_addr_copy(&sin6->sin6_addr, &pki->ipi6_addr);
>>          break;
>>          }
>>      }
>
> Neil asked me to add macros for casting sockaddr_storage fields in the 
> svc_rqst structure (see svc_addr() and svc_addr_in()).  You might 
> consider adding similar macros for cleanly extracting address 
> information from svc_xprt.
>
That's a good idea. We only have svc_local_port now, but could add more.
>> @@ -506,7 +522,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
>>      struct cmsghdr *cmh = &buffer.hdr;
>>      int        err, len;
>>      struct msghdr msg = {
>> -        .msg_name = svc_addr(rqstp),
>> +        .msg_name = &svsk->sk_xprt.xpt_remote,
>>          .msg_control = cmh,
>>          .msg_controllen = sizeof(buffer),
>>          .msg_flags = MSG_DONTWAIT,
>> @@ -541,7 +557,10 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
>>          svc_xprt_received(&svsk->sk_xprt);
>>          return -EAGAIN;
>>      }
>> -    rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
>> +    len = svc_sockaddr_len((struct sockaddr 
>> *)&svsk->sk_xprt.xpt_remote);
>> +    if (len < 0)
>> +        return len;
>> +    svsk->sk_xprt.xpt_remotelen = len;
>>      if (skb->tstamp.tv64 == 0) {
>>          skb->tstamp = ktime_get_real();
>>          /* Don't enable netstamp, sunrpc doesn't


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC,PATCH] svc: REPOST - Fix the UDP address logic

[Greg Banks]

On Wed, Oct 24, 2007 at 02:29:59PM -0400, Chuck Lever wrote:
> Tom Tucker wrote:
> > 
> >+static size_t svc_sockaddr_len(struct sockaddr *addr)
> >+{
> >+	switch (addr->sa_family) {
> >+	case AF_INET:
> >+		return sizeof(struct sockaddr_in);
> >+	case AF_INET6:
> >+		return sizeof(struct sockaddr_in6);
> >+	}
> >+	dprintk("svc: unrecognized address family %d\n", addr->sa_family);
> >+	return -ENOTSUPP;
> >+}
> >+
> 
> Nit: sa_family is probably an unsigned short, so use "%u" instead of
> "%d".  Also I'm using -EAFNOSUPPORT in similar error cases, but I may 
> have missed a stated preference.

EAFNOSUPPORT is the most appropriate error.

Also, you might want to make the return the signed ssize_t so that
the return value -EAFNOSUPPORT is actually negative.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs