Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <c7c62df7-74bb-416f-b363-274c96b88ae0@redhat.com>
Date:   Sat, 2 Dec 2023 17:39:36 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] locking: Document that mutex_unlock() is non-atomic
Content-Language: en-US
To:     David Laight <David.Laight@ACULAB.COM>,
        Jann Horn <jannh@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
References: <20231130204817.2031407-1-jannh@google.com>
 <06c05c8b-9a3b-4c04-b898-0f82e98da70f@redhat.com>
 <CAG48ez1a=VuEWwPTjcXFAwCyt9bRH-WzAfw0uP-qVu83kdxkZw@mail.gmail.com>
 <28b147c3d7354d1a8ff0b903da9b54f4@AcuMS.aculab.com>
 <CAG48ez1bOwbqEbD_ycC2fyWK_tW4rqr=gogJbQOx5CECyPGZcg@mail.gmail.com>
 <1bcee696-d751-413c-a2ec-4a8480bae00b@redhat.com>
 <780e652ff52044d4a213cacbd9276cf8@AcuMS.aculab.com>
 <501501ef-137a-4797-9d43-48ea68851147@redhat.com>
 <f98b04e05c964b57bc27fb895810aaaf@AcuMS.aculab.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <f98b04e05c964b57bc27fb895810aaaf@AcuMS.aculab.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/2/23 10:51, David Laight wrote:
> From: Waiman Long
>> Sent: 01 December 2023 19:16
>>
>> On 12/1/23 13:44, David Laight wrote:
>>> Pending waiters aren't the problem.
>>>
>> Pending waiters can still be a problem if code decides to free the lock
>> containing object after a lock/unlock sequence as it may cause
>> use-after-free.
>>> You have to ensure there aren't any, but the mutex() can be held.
>>>
>> Using reference count to track the number of active users is one way to
>> prevent that if you only release the reference count after
>> mutex_unlock() returns but not in the lock critical section.
> I suspect the documentation need to be more explicit than just saying
> it is non-atomic.
> Saying something like:
>
> The mutex structure may be accessed by mutex_unlock() after another
> thread has locked and unlocked the mutex.
>
> So if a reference count is used to ensure a structure remains valid when
> a lock is released (with the item being freed when the count becomes zero)
> the reference count itself cannot be protected by a mutex in the structure.
> So code like:
> 	...
> 	count = --item->refcount;
> 	mutex_unlock(item->mtx);
> 	if (!count)
> 		free(item);
> can lead to a 'use after free' in mutex_unlock().
> However if the refcount is atomic and decremented without the
> mutex held there isn't a problem.
>
> 	David

That is definitely better than saying it is non-atomic which is vague in 
meaning.

Cheers,
Longman