linux-kernel - Re: [patch V2 3/4] atomics: Provide rcuref

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Mar 2023 17:05:56 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Qiuxu Zhuo <qiuxu.zhuo@...el.com>
Cc:     arjan.van.de.ven@...el.com, arjan@...ux.intel.com,
        boqun.feng@...il.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, linux-kernel@...r.kernel.org,
        mark.rutland@....com, maz@...nel.org, netdev@...r.kernel.org,
        pabeni@...hat.com, peterz@...radead.org,
        torvalds@...uxfoundation.org, wangyang.guo@...el.com,
        will@...nel.org, x86@...nel.org
Subject: Re: [patch V2 3/4] atomics: Provide rcuref - scalable reference
 counting

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