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

[Greg Banks <gnb@sgi.com>]

Chuck Lever wrote:
> On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote:
>
>>>
>>> @@ -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.
Just a bit. The svcxprt_rdma, the rdma_cm_id, a bunch of other IB state...
>>
>> 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.

My 2c. If I understand the RDMA CM behaviour correctly, it should not be
possible at this point in the code for
newxprt->sc_cm_id->route.addr.{src,dst}_addr to be anything but a valid
sockaddr_storages holding the IPv4 or (theoretically) the IPv6 address
of the server and client. So I would be happy with a BUG_ON(len < 0).
That would also render the leakage moot.

-- 
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.