Re: [PATCH] console: generalize the ability for domU access

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2023 18:58, Daniel P. Smith wrote:
> On 8/4/23 03:49, Jan Beulich wrote:
>> On 03.08.2023 18:31, Daniel P. Smith wrote:
>>> On 8/3/23 11:56, Jan Beulich wrote:
>>>> On 03.08.2023 14:56, Daniel P. Smith wrote:
>>>>> On 8/2/23 07:01, Jan Beulich wrote:
>>>>>> On 01.08.2023 18:06, Daniel P. Smith wrote:
>>>>>>> +        {
>>>>>>> +            for_each_domain(next)
>>>>>>
>>>>>> What guarantees that the list won't change behind your back? You don't
>>>>>> hold domlist_read_lock here afaict. It might be that you're safe because
>>>>>> that lock is an RCU one and this function is only invoked at init time
>>>>>> or from some form of interrupt handler. But that's far from obvious and
>>>>>> will hence need both properly confirming and stating in a comment. (It
>>>>>> is actually this concern, iirc, which so far had us avoid iterating the
>>>>>> domain list here.)
>>>>>
>>>>> It is better to error on the side of caution instead of assuming this
>>>>> will always be invoked in a safe manner. I will add a read lock for the
>>>>> domain list.
>>>>
>>>> I'm not firm enough in RCU to be certain whether acquiring that lock is
>>>> permissible here.
>>>
>>> Same and I took your statements to suggest that I should.
>>
>> Actually I wasn't paying close enough attention here: The code already
>> uses rcu_lock_domain_by_id(), which acquires domlist_read_lock.
>>
> 
> Right, it grabs the lock while iterating through domain_hash[], I 
> thought your concern was with regard to the iterating with 
> for_each_domain and the embedded open coded version. Because of your 
> inquiry, I have been thinking about it and I should be grabbing the lock 
> as I iterate to be sure that I don't get deceived into believing the end 
> of list was hit because a domain was being removed as I walked the list. 
> And if it so happens that the context is always safe, then there should 
> be no contention on grabbing the lock. Do you disagree?

Well, RCU locks aren't real locks, so there's no contention in the first
place. As to the context being safe - I continue to be uncertain, but
the pre-existing use of the lock means you adding the necessary locking
to your code won't regress in that regard.

Jan