Re: [PATCH for-next 1/6] RDMA/core: Use refcount_t instead of atomic_t for reference counting

[liweihang <liweihang@huawei.com>]

On 2021/5/18 0:56, Jason Gunthorpe wrote:
> On Sat, May 15, 2021 at 03:07:58AM +0000, liweihang wrote:
>> On 2021/5/14 20:34, Jason Gunthorpe wrote:
>>> On Fri, May 14, 2021 at 10:11:34AM +0800, Weihang Li wrote:
>>>> The refcount_t API will WARN on underflow and overflow of a reference
>>>> counter, and avoid use-after-free risks. Increase refcount_t from 0 to 1 is
>>>> regarded as there is a risk about use-after-free. So it should be set to 1
>>>> directly during initialization.
>>>
>>> What does this comment about 0 to 1 mean?
>>>
>>
>> Hi Jason,
>>
>> I first thought refcount_inc() and atomic_inc() are exactly the same, but I got
>> a warning about refcount_t on iwpm_init() after the replacement:
>>
>> [   16.882939] refcount_t: addition on 0; use-after-free.
>> [   16.888065] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25
>> refcount_warn_saturate+0xa0/0x144
>> ...
>> [   17.014698] Call trace:
>> [   17.017135]  refcount_warn_saturate+0xa0/0x144
>> [   17.021559]  iwpm_init+0x104/0x12c
>> [   17.024948]  iw_cm_init+0x24/0xd0
>> [   17.028248]  do_one_initcall+0x54/0x2d0
>> [   17.032068]  kernel_init_freeable+0x224/0x294
>> [   17.036407]  kernel_init+0x20/0x12c
>> [   17.039880]  ret_from_fork+0x10/0x18
>>
>> Then I noticed that the comment of refcount_inc() says:
>>
>>  * Will WARN if the refcount is 0, as this represents a possible use-after-free
>>  * condition.
>>
>> so I made changes:
>>
>> @@ -77,8 +77,12 @@ int iwpm_init(u8 nl_client)
>>                         ret = -ENOMEM;
>>                         goto init_exit;
>>                 }
>> +
>> +               refcount_set(&iwpm_admin.refcount, 1);
>> +       } else {
>> +               refcount_inc(&iwpm_admin.refcount);
>>         }
>> -       refcount_inc(&iwpm_admin.refcount);
>> +
>>
>> I wrote the comments because I thought someone might be confused by the above
>> changes :)
> 
> Stuff like this needs to be split into a single patch for iwpm_admin
> 
> Jason
> 

Sure, I will split all of the changes into separate patches.

Weihang