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 :)
> This all seems like a good idea but I wish you had done one patch per
> variable changed
>
> Jason
>
Sure, thanks.
Weihang
