Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC)
 interception handler
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     jjherne@linux.ibm.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, borntraeger@de.ibm.com,
        cohuck@redhat.com, pasic@linux.vnet.ibm.com,
        alex.williamson@redhat.com, kwankhede@nvidia.com,
        frankja@linux.ibm.com, david@redhat.com, imbrenda@linux.ibm.com,
        hca@linux.ibm.com
References: <20210521193648.940864-1-akrowiak@linux.ibm.com>
 <20210521193648.940864-3-akrowiak@linux.ibm.com>
 <5d15fdf2-aee8-4e6c-c3e1-f07c76ce5974@linux.ibm.com>
 <e2bed0a6-f5e2-0a69-22b9-1b304cbe1362@linux.ibm.com>
 <20210525131912.GW1002214@nvidia.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
Message-ID: <c54ef522-f348-df16-a99f-1e31feb1b0bd@linux.ibm.com>
Date:   Tue, 25 May 2021 11:56:50 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20210525131912.GW1002214@nvidia.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


> Why can't you put the locks in the right order? It looked trivial, I'm confused.
>
> Jason

I explained this in one of my responses to the previous series. Maybe I 
didn't do
a good job of it, so let me see if I can provide a more thorough 
explanation.

The handle_pqap() function in priv.c does not have any access to the
matrix_dev->lock which lives in the vfio_ap module, so there is no
way for this function to control the order of locking. The only lock
it can access is the lock provided for the hook function pointer which
must be held for the duration of the execution of the hook. When the
hook function (handle_pqap() in vfio_ap_ops.c) is called, it has to lock
the matrix_dev->lock mutex to do its thing. The interception of the
PQAP instruction that sets off the above scenario can happen simultaneously
with both the vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm()
instructions in vfio_ap_ops.c.

The vfio_ap_mdev_set_kvm() function is called only by the group notifier
callback when the vfio_ap driver is notified that the KVM pointer has 
been set.
In this case, we could set the lock for the hook function before setting
the matrix_dev->lock and calling the vfio_ap_mdev_set_kvm() function and
all would be well.

The vfio_ap_mdev_unset_kvm() function, however, is called both by the group
notifier when the KVM pointer has been cleared or when the mdev is
being removed. In both cases, the only way to get the KVM pointer - which
is needed to unplug the AP resources from the guest - is from the 
matrix_mdev
which contains it. This, of course, needs to be done while holding the
matrix_dev->lock mutex. The vfio_ap_mdev_unset_kvm() function also
clears the hook function pointer, but can only get the lock used to 
control access
to it from the matrix_mdev; therein lies the rub. So we can have the 
following
scenario which is flagged by lockdep:

CPU x:                                            CPU y:
--------                                             --------
                                                       lock the 
matrix_dev->lock:
vfio_ap_mdev_set_kvm in vfio_ap_ops.c

lock the hook pointer:
handle_pqap in priv.c

lock the matrix_dev->lock
handle_pqap in vfio_ap_ops.c

                                                       lock the hook 
pointer:
vfio_ap_mdev_set_kvm in vfio_ap_ops.c


Maybe I'm missing something, but I was unable to find a way around this when
the hook function pointer and its locking mechanism is stored in a field 
of a satellite
structure of struct kvm.