linux-kernel - Re: [PATCH v4 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210319005953.7e6425bd.pasic@linux.ibm.com>
Date:   Fri, 19 Mar 2021 00:59:53 +0100
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, stable@...r.kernel.org,
        borntraeger@...ibm.com, cohuck@...hat.com, kwankhede@...dia.com,
        pbonzini@...hat.com, alex.williamson@...hat.com,
        pasic@...ux.vnet.ibm.com
Subject: Re: [PATCH v4 1/1] s390/vfio-ap: fix circular lockdep when
 setting/clearing crypto masks

On Thu, 18 Mar 2021 14:38:53 -0400
Tony Krowiak <akrowiak@...ux.ibm.com> wrote:

> On 3/17/21 7:17 PM, Halil Pasic wrote:
> > On Wed, 10 Mar 2021 10:05:59 -0500
> > Tony Krowiak <akrowiak@...ux.ibm.com> wrote:
> >  
> >> -		ret = vfio_ap_mdev_reset_queues(mdev);
> >> +		matrix_mdev = mdev_get_drvdata(mdev);  
> > Is it guaranteed that matrix_mdev can't be NULL here? If yes, please
> > remind me of the mechanism that ensures this.
> >  
> >> +
> >> +		/*
> >> +		 * If the KVM pointer is in the process of being set, wait until
> >> +		 * the process has completed.
> >> +		 */
> >> +		wait_event_cmd(matrix_mdev->wait_for_kvm,
> >> +			       matrix_mdev->kvm_busy == false,
> >> +			       mutex_unlock(&matrix_dev->lock),
> >> +			       mutex_lock(&matrix_dev->lock));
> >> +
> >> +		if (matrix_mdev->kvm)
> >> +			ret = vfio_ap_mdev_reset_queues(mdev);
> >> +		else
> >> +			ret = -ENODEV;  
> > Didn't we agree to make the call to vfio_ap_mdev_reset_queues()
> > unconditional again (for reference please take look at
> > Message-ID: <64afa72c-2d6a-2ca1-e576-34e15fa579ed@...ux.ibm.com>)?  
> 
> How about this:

Looks good. I will check the mdev code if the checkeck is really
needed. I'm curious when the sysfs files associated with a new mdev are
created. My guess is that this one comes in via a device specific file
(not the parent like in case of the create), and that those may be
created after the create. But we can get rid of the check any time so I
really don't see it as something that would preclude merging this.

Regards,
Halil

> 
> static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
>                      unsigned int cmd, unsigned long arg)
> {
>      int ret = 0;
>      struct ap_matrix_mdev *matrix_mdev;
> 
>      ...
>      case VFIO_DEVICE_RESET:
>          matrix_mdev = mdev_get_drvdata(mdev);
>          WARN(!matrix_mdev, "Driver data missing from mdev!!");
> 
>          if (matrix_mdev) {
>              /*
>               * If the KVM pointer is in the process of being set, wait 
> until
>               * the process has completed.
>               */
>              wait_event_cmd(matrix_mdev->wait_for_kvm,
>                         matrix_mdev->kvm_busy == false,
> mutex_unlock(&matrix_dev->lock),
> mutex_lock(&matrix_dev->lock));
> 
>              ret = vfio_ap_mdev_reset_queues(mdev);
>          }
>          break;
>      ...
> 
>      return ret;
> }
> 
> >
> > Regards,
> > Halil  
>