Re: [PATCH 01/19] SUNRPC: Fix error return value of svc_addr_len()

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Apr 28, 2009 at 11:28:47AM -0400, Chuck Lever wrote:
>
> On Apr 27, 2009, at 7:51 PM, J. Bruce Fields wrote:
>
>> On Mon, Apr 27, 2009 at 12:49:07PM -0400, Chuck Lever wrote:
>>> On Apr 25, 2009, at 6:17 PM, J. Bruce Fields wrote:
>>>> On Thu, Apr 23, 2009 at 07:31:25PM -0400, Chuck Lever wrote:
>>>>> The svc_addr_len() helper function returns -EAFNOSUPPORT if it
>>>>> doesn't
>>>>> recognize the address family of the passed-in socket address.
>>>>> However,
>>>>> the return type of this function is size_t, which means - 
>>>>> EAFNOSUPPORT
>>>>> is turned into a very large positive value in this case.
>>>>>
>>>>> The check in svc_udp_recvfrom() to see if the return value is less
>>>>> than zero therefore won't work at all.
>>>>>
>>>>> Additionally, handle_connect_req() passes this value directly to
>>>>> memset().  This could cause memset() to clobber a large chunk of
>>>>> memory
>>>>> if svc_addr_len() has returned an error.  Currently the address
>>>>> family
>>>>> of these addresses, however, is known to be supported long before
>>>>> handle_connect_req() is called, so this isn't a real risk.
>>>>>
>>>>> Change the error return value of svc_addr_len() to zero, which fits
>>>>> in
>>>>> the range of size_t, and is safer to pass to memset() directly.
>>>>>
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>
>>>>> include/linux/sunrpc/svc_xprt.h |    5 +++--
>>>>> net/sunrpc/svcsock.c            |    7 ++++---
>>>>> 2 files changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/ 
>>>>> sunrpc/
>>>>> svc_xprt.h
>>>>> index 0d9cb6e..d790c52 100644
>>>>> --- a/include/linux/sunrpc/svc_xprt.h
>>>>> +++ b/include/linux/sunrpc/svc_xprt.h
>>>>> @@ -118,7 +118,7 @@ static inline unsigned short  
>>>>> svc_addr_port(const
>>>>> struct sockaddr *sa)
>>>>> 	return 0;
>>>>> }
>>>>>
>>>>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>>>>> +static inline size_t svc_addr_len(const struct sockaddr *sa)
>>>>> {
>>>>> 	switch (sa->sa_family) {
>>>>> 	case AF_INET:
>>>>> @@ -126,7 +126,8 @@ static inline size_t svc_addr_len(struct
>>>>> sockaddr *sa)
>>>>> 	case AF_INET6:
>>>>> 		return sizeof(struct sockaddr_in6);
>>>>> 	}
>>>>> -	return -EAFNOSUPPORT;
>>>>> +
>>>>
>>>> May as well stick a WARN() here too if only as a shorthand way of
>>>> documenting that this isn't meant to happen.
>>>
>>> Sounds reasonable, but:
>>>
>>> 1.  I thought BUG() was the way to document "shouldn't happen," and
>>
>> That'd be OK too.
>>
>>>
>>> 2.  What about all the other static inliney places this "shouldn't
>>> happen" ?
>>
>> Such as?
>
> nlm_cmp_addr(), nlm_privileged_requester(), svc_addr_port(),  
> nfs_sockaddr_match_ipaddr(), nfs_sockaddr_cmp(), nfs_server_get_key(),  
> nfs_set_port(), nfs_verify_server_address(), and so on.

I see.  OK, I'm fine with leaving it as is.--b.