Re: [PATCH net-next 2/3] net: sctp: sctp_association_init: hold refs in reverse order

[Daniel Borkmann <dborkman@redhat.com>]

On 06/07/2013 01:00 PM, Neil Horman wrote:
> On Fri, Jun 07, 2013 at 10:35:05AM +0200, Daniel Borkmann wrote:
>> In case we need to bail out for whatever reason during assoc
>> init, we call sctp_endpoint_put() and then sock_put(), however,
>> we've hold both refs in reverse order, so first sctp_endpoint_hold()
>> and then sock_hold(). Reverse this, so that we have sock_hold()
>> with sctp_endpoint_hold() first and then in error case
>> sctp_endpoint_put() and then sock_put(). Actually shouldn't
>> matter much since we just increase an atomic, but that way, it's
>> more clean.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   net/sctp/associola.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 91cfd8f..04795fb 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -86,11 +86,10 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>>
>>   	/* Discarding const is appropriate here.  */
>>   	asoc->ep = (struct sctp_endpoint *)ep;
>> -	sctp_endpoint_hold(asoc->ep);
>> -
>> -	/* Hold the sock.  */
>>   	asoc->base.sk = (struct sock *)sk;
>> +
>>   	sock_hold(asoc->base.sk);
>> +	sctp_endpoint_hold(asoc->ep);
>>
>>   	/* Initialize the common base substructure.  */
>>   	asoc->base.type = SCTP_EP_TYPE_ASSOCIATION;
>
> This looks good, but you may want to instead reverse the order in which we do
> the puts at fail_init, as other call sites that hold both endpoint socket do so
> in endpoint, sock order, and it would probably be nice to be consistent in that
> order.

Thanks, will do. When we have clarified the 1st patch, I'll simply respin the
entire patchset and send a v2.