Re: [patch V2 3/4] atomics: Provide rcuref - scalable reference counting

[Thomas Gleixner]

Qiuxu!

On Thu, Mar 09 2023 at 16:35, Qiuxu Zhuo wrote:

>> rcuref treats the underlying atomic_t as an unsigned integer and partitions
>> this space into zones:
>> 
>>   0x00000000 - 0x7FFFFFFF	valid zone (1 .. INT_MAX references)
>
> From the point of rcuref_read()'s view:
> 0x00000000 encodes 1, ...,  then 0x7FFFFFFF should encode INT_MAX + 1
> references.

orrect.

>> + * The actual race is possible due to the unconditional increment and
>> + * decrements in rcuref_get() and rcuref_put():
>> + *
>> + *	T1				T2
>> + *	get()				put()
>> + *					if (atomic_add_negative(1, &ref->refcnt))
>
> For T2 put() here:
> "if (atomic_add_negative(1, &ref->refcnt))" ->
> "if (atomic_add_negative(-1, &ref->refcnt))"

Yup.


>> + *		succeeds->			atomic_cmpxchg(&ref->refcnt, -1, DEAD);
>
> Is it more readable if 's/-1/NODEF/g' ?

True

>> + *	T1				T2
>> + *	put()				get()
>> + *	// ref->refcnt = ONEREF
>> + *	if (atomic_add_negative(-1, &ref->cnt))
>
> For T1 put() here:
> "if (atomic_add_negative(-1, &ref->cnt))" ->
> "if (!atomic_add_negative(-1, &ref->cnt))"

Indeed.

>> + *		return false;				<- Not taken
>> + *
>> + *	// ref->refcnt == NOREF
>> + *	--> preemption
>> + *					// Elevates ref->c to ONEREF
>
> s/ref->c/ref->refcnt/g

Yes.

>> + *					if (!atomic_add_negative(1, &ref->refcnt))
>> + *						return true;			<- taken
>> + *
>> + *					if (put(&p->ref)) { <-- Succeeds
>> + *						remove_pointer(p);
>> + *						kfree_rcu(p, rcu);
>> + *					}
>> + *
>> + *		RCU grace period ends, object is freed
>> + *
>> + *	atomic_cmpxchg(&ref->refcnt, NONE, DEAD);	<- UAF
>
> s/NONE/NOREF/g

Right. Thanks for spotting these details!

       Thomas