From:   Thomas Gleixner <tglx@linutronix.de>
To:     Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Cc:     arjan.van.de.ven@intel.com, arjan@linux.intel.com,
        boqun.feng@gmail.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, maz@kernel.org, netdev@vger.kernel.org,
        pabeni@redhat.com, peterz@infradead.org,
        torvalds@linuxfoundation.org, wangyang.guo@intel.com,
        will@kernel.org, x86@kernel.org
Subject: Re: [patch V2 3/4] atomics: Provide rcuref - scalable reference
 counting
In-Reply-To: <20230309083523.66592-1-qiuxu.zhuo@intel.com>
References: <20230307125538.932671660@linutronix.de>
 <20230309083523.66592-1-qiuxu.zhuo@intel.com>
Date:   Mon, 20 Mar 2023 17:05:56 +0100
Message-ID: <87o7on9zmj.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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