Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len()

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

On Tue, Mar 24, 2009 at 04:37:49PM -0400, Chuck Lever wrote:
> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>> On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote:
>>> The svc_addr_len() helper function can return a negative errno value,
>>> but its return type is size_t, which is unsigned.
>>>
>>> The RDMA transport code passes this return value directly to  
>>> memset(),
>>> without checking first if it's negative.  This could cause memset()  
>>> to
>>> clobber a large piece of memory if svc_addr_len() has returned an
>>> error.
>>
>> I'd still like to understand when this can happen, to better  
>> understand
>> how the error should be handled.
>>
>> Also:
>>
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> include/linux/sunrpc/svc_xprt.h          |    3 ++-
>>> net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
>>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/ 
>>> svc_xprt.h
>>> index 0127dac..c2aa8cd 100644
>>> --- a/include/linux/sunrpc/svc_xprt.h
>>> +++ b/include/linux/sunrpc/svc_xprt.h
>>> @@ -113,7 +113,7 @@ static inline unsigned short  
>>> svc_addr_port(struct sockaddr *sa)
>>> 	return ret;
>>> }
>>>
>>> -static inline size_t svc_addr_len(struct sockaddr *sa)
>>> +static inline int svc_addr_len(const struct sockaddr *sa)
>>> {
>>> 	switch (sa->sa_family) {
>>> 	case AF_INET:
>>> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct  
>>> sockaddr *sa)
>>> 	case AF_INET6:
>>> 		return sizeof(struct sockaddr_in6);
>>> 	}
>>> +
>>> 	return -EAFNOSUPPORT;
>>> }
>
> It's clearly a bug to return -EAFNOSUPPORT in a size_t.

I agree.  My comment is just that this patch doesn't necessarily fix the
real bug; so as long as there's a bug here, I'd prefer it to be an
obvious one.

>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/ 
>>> xprtrdma/svc_rdma_transport.c
>>> index 3d810e7..d1ec6f9 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id 
>>> *new_cma_id, size_t client_ird)
>>> 	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>>> 	struct svcxprt_rdma *newxprt;
>>> 	struct sockaddr *sa;
>>> +	int len;
>>>
>>> 	/* Create a new transport */
>>> 	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
>>> @@ -563,9 +564,20 @@ static void handle_connect_req(struct  
>>> rdma_cm_id *new_cma_id, size_t client_ird)
>>>
>>> 	/* Set the local and remote addresses in the transport */
>>> 	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
>>> -	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
>>> +	len = svc_addr_len(sa);
>>> +	if (len < 0) {
>>> +		dprintk("svcrdma: dst_addr has a bad address family\n");
>>> +		return;
>>
>> we're probably leaking something here.
>>
>> I don't want to fix this until it's understood well enough to fix it
>> correctly.
>
> Tom needs respond to this, but I think it would be safe to simply BUG() 
> here.

Could be.--b.