Re: [PATCH] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 7/13/21 12:45 PM, Halil Pasic wrote:
> On Tue, 13 Jul 2021 09:48:01 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> On 7/12/21 7:38 PM, Halil Pasic wrote:
>>> On Wed,  7 Jul 2021 11:41:56 -0400
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>   
>>>> It was pointed out during an unrelated patch review that locks should not
>>>> be open coded - i.e., writing the algorithm of a standard lock in a
>>>> function instead of using a lock from the standard library. The setting and
>>>> testing of the kvm_busy flag and sleeping on a wait_event is the same thing
>>>> a lock does. Whatever potential deadlock was found and reported via the
>>>> lockdep splat was not magically removed by going to a wait_queue; it just
>>>> removed the lockdep annotations that would identify the issue early
>>> Did you change your opinion since we last talked about it? This reads to
>>> me like we are deadlocky without this patch, because of the last
>>> sentence.
>> The words are a direct paraphrase of Jason G's responses to my
>> query regarding what he meant by open coding locks. I
>> am choosing to take his word on the subject and remove the
>> open coded locks.
>>
>> Having said that, we do not have a deadlock problem without
>> this patch. If you recall, the lockdep splat occurred ONLY when
>> running a Secure Execution guest in a CI environment. Since
>> AP is not yet supported for SE guests, there is no danger of
>> a lockdep splat occurring in a customer environment. Given
>> Jason's objections to the original solution (i.e., kvm_busy flag
>> and wait queue), I decided to replace the so-called open
>> coded locks.
> I'm in favor of doing that. But if   ("s390/vfio-ap: fix
> circular lockdep when setting/clearing crypto masks") ain't buggy,
> then this patch does not qualify for stable. For a complete set of
> rules consult:
> https://github.com/torvalds/linux/blob/master/Documentation/process/stable-kernel-rules.rst
>
> Here the most relevant points:
> * It must fix a real bug that bothers people (not a, "This could be a
> problem..." type thing).
> * t must fix a problem that causes a build error (but not for things
> marked CONFIG_BROKEN), an oops, a hang, data corruption, a real security
> issue, or some "oh, that's not good" issue. In short, something critical.
> * No "theoretical race condition" issues, unless an explanation of how
> the race can be exploited is also provided.
>
> Jason may give it another try to convince us that 0cc00c8d4050 only
> silenced lockdep, but vfio_ap remained prone to deadlocks. To my best
> knowledge using condition variable and a mutex is one of the well known
> ways to implement an rwlock.
>
> In my opinion, you should drop the fixes tag, drop the cc stable, and
> provide a patch description that corresponds to *your* understanding
> of the situation.

I'll drop the fixes and cc stable. Given the patch was created in
response to Jason G's comments - which are paraphrased in
the patch description - the patch description corresponds
directly to my understanding of the situation. It is precisely why
I created the patch.

>
> Neither the Fixes tag or the stable process is (IMHO) meant for these
> types of (style) issues. And if you don't think the alleged problem is
> real, don't make the description of your patch say it is real.
> Regards,
> Halil
>
>>> Regards,
>>> Halil