Re: [PATCH v5 7/8] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche <bvanassche@acm.org>]

On 10/12/26 5:24 AM, Christoph Hellwig wrote:
> On Thu, Oct 08, 2026 at 03:42:18PM -0700, Bart Van Assche wrote:
>>   static inline void
>>   sdeb_meta_read_lock(struct sdeb_store_info *sip)
>> +	__acquires_shared(&sip->macc_meta_lck)
>> +	__context_unsafe(/*conditional locking*/)
>>   {
>> -	if (sdebug_no_rwlock) {
>> -		if (sip)
>> -			__acquire(&sip->macc_meta_lck);
>> -		else
>> -			__acquire(&sdeb_fake_rw_lck);
>> -	} else {
>> +	if (!sdebug_no_rwlock) {
> 
> Why can't this be modelled using a __acquire variant?  And if you
> add __context_unsafe(/*conditional locking*/) please add a detailed
> comment why that is needed, otherwise no one is going to understand
> this in the future.

Hi Christoph,

I will change the comment into "can't be annotated with
__{acquire,release}(,_shared}() without triggering undefined behavior".

What Sashiko reported on a Google-internal server for an
earlier attempt to annotate this and similar functions with
__{acquire,release}(,_shared}() has been included below. I think that
Sashiko is right:
=======================================================================
 >  static inline void
 >  sdeb_meta_read_lock(struct sdeb_store_info *sip)
 > +	__acquires_shared(&sip->macc_meta_lck)
 >  {
 >  	if (sdebug_no_rwlock) {
 > -		if (sip)
 > -			__acquire(&sip->macc_meta_lck);
 > -		else
 > -			__acquire(&sdeb_fake_rw_lck);
 > +		__acquire_shared(&sip->macc_meta_lck);
 >  	} else {
 > -		if (sip)
 > +		if (sip) {
 >  			read_lock(&sip->macc_meta_lck);
 > -		else
 > +		} else {
 >  			read_lock(&sdeb_fake_rw_lck);
 > +			__release_shared(&sdeb_fake_rw_lck);
 > +			__acquire_shared(&sip->macc_meta_lck);
                                          ^^^^^^^^^^^^^^^^^^^
Can this regression trigger an undefined behavior splat when sip is NULL?
When the scsi_debug driver is used with the module parameter fake_rw=1,
devip2sip() returns NULL. In this else branch, sip is explicitly NULL,
so evaluating &sip->macc_meta_lck means taking the address of a member
from a NULL pointer.
When compiled with Context Analysis and UBSAN (-fsanitize=null or
-fsanitize=pointer-overflow), the inline function call evaluates the
argument before the call and will trigger a runtime splat. Without UBSAN,
it calculates an invalid offset from a NULL pointer.
=======================================================================

Bart.