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;
Date:   Thu, 11 Feb 2021 17:47:50 +0100
From:   Halil Pasic <pasic@linux.ibm.com>
To:     Tony Krowiak <akrowiak@linux.ibm.com>
Cc:     Cornelia Huck <cohuck@redhat.com>, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        stable@vger.kernel.org, borntraeger@de.ibm.com,
        kwankhede@nvidia.com, pbonzini@redhat.com,
        alex.williamson@redhat.com, pasic@linux.vnet.ibm.com
Subject: Re: [PATCH 1/1] s390/vfio-ap: fix circular lockdep when
 setting/clearing crypto masks
Message-ID: <20210211174750.24d91a65.pasic@linux.ibm.com>
In-Reply-To: <8c461602-8c2c-4dd9-1d2b-5e424fc701f8@linux.ibm.com>
References: <20210209194830.20271-1-akrowiak@linux.ibm.com>
        <20210209194830.20271-2-akrowiak@linux.ibm.com>
        <20210210115334.46635966.cohuck@redhat.com>
        <20210210162429.261fc17c.pasic@linux.ibm.com>
        <20210210163237.315d9a68.pasic@linux.ibm.com>
        <59e8f084-c9ec-ce25-2326-b206e30d04d0@linux.ibm.com>
        <20210210234606.1d0dbdec.pasic@linux.ibm.com>
        <8c461602-8c2c-4dd9-1d2b-5e424fc701f8@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 11 Feb 2021 09:21:26 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> Yes, it makes sense. I guess I didn't look closely at your
> suggestion when I said it was exactly what I implemented
> after agreeing with Connie. I had a slight difference in
> my implementation:
> 
> static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> {
>      struct kvm *kvm;
> 
>      mutex_lock(&matrix_dev->lock);
> 
>      if (matrix_mdev->kvm) {
>          kvm = matrix_mdev->kvm;
>          mutex_unlock(&matrix_dev->lock);

The problem with this one is that as soon as we drop
the lock here, another thread can in theory execute
the critical section below, which drops our reference
to kvm via kvm_put_kvm(kvm). Thus when we enter
kvm_arch_crypto_clear_mask(), even if we are guaranteed
to have a non-null pointer, the pointee is not guaranteed
to be around. So like Connie suggested, you better take
another reference to kvm in the first critical section.

Regards,
Halil

>          kvm_arch_crypto_clear_masks(kvm);
>          mutex_lock(&matrix_dev->lock);
>          kvm->arch.crypto.pqap_hook = NULL;
>          vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
>          matrix_mdev->kvm = NULL;
>          kvm_put_kvm(kvm);
>      }
> 
>      mutex_unlock(&matrix_dev->lock);
> }