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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201222204335.1b456342.pasic@linux.ibm.com>
Date:   Tue, 22 Dec 2020 20:43:35 +0100
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     Tony Krowiak <akrowiak@...ux.ibm.com>, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        stable@...r.kernel.org, borntraeger@...ibm.com,
        kwankhede@...dia.com, pbonzini@...hat.com,
        alex.williamson@...hat.com, pasic@...ux.vnet.ibm.com
Subject: Re: [PATCH v4] s390/vfio-ap: clean up vfio_ap resources when KVM
 pointer invalidated

On Tue, 22 Dec 2020 16:57:06 +0100
Cornelia Huck <cohuck@...hat.com> wrote:

> On Tue, 22 Dec 2020 10:37:01 -0500
> Tony Krowiak <akrowiak@...ux.ibm.com> wrote:
> 
> > On 12/21/20 11:05 PM, Halil Pasic wrote:  
> > > On Mon, 21 Dec 2020 13:56:25 -0500
> > > Tony Krowiak <akrowiak@...ux.ibm.com> wrote:  
> 
> > >>   static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> > >>   				       unsigned long action, void *data)
> > >>   {
> > >> -	int ret;
> > >> +	int ret, notify_rc = NOTIFY_DONE;
> > >>   	struct ap_matrix_mdev *matrix_mdev;
> > >>   
> > >>   	if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> > >>   		return NOTIFY_OK;
> > >>   
> > >>   	matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
> > >> +	mutex_lock(&matrix_dev->lock);
> > >>   
> > >>   	if (!data) {
> > >> -		matrix_mdev->kvm = NULL;
> > >> -		return NOTIFY_OK;
> > >> +		if (matrix_mdev->kvm)
> > >> +			vfio_ap_mdev_unset_kvm(matrix_mdev);
> > >> +		notify_rc = NOTIFY_OK;
> > >> +		goto notify_done;
> > >>   	}
> > >>   
> > >>   	ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
> > >>   	if (ret)
> > >> -		return NOTIFY_DONE;
> > >> +		goto notify_done;
> > >>   
> > >>   	/* If there is no CRYCB pointer, then we can't copy the masks */
> > >>   	if (!matrix_mdev->kvm->arch.crypto.crycbd)
> > >> -		return NOTIFY_DONE;
> > >> +		goto notify_done;
> > >>   
> > >>   	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
> > >>   				  matrix_mdev->matrix.aqm,
> > >>   				  matrix_mdev->matrix.adm);
> > >>   
> > >> -	return NOTIFY_OK;    
> > > Shouldn't there be an
> > >   +	notify_rc = NOTIFY_OK;
> > > here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch
> > > on success you set notify_rc to NOTIFY_OK, but in the !!data branch it
> > > just stays NOTIFY_DONE. Or am I missing something?    
> > 
> > I don't think it matters much since NOTIFY_OK and NOTIFY_DONE have
> > no further effect on processing of the notification queue, but I believe
> > you are correct, this is a change from what we originally had. I can
> > restore the original return values if you'd prefer.  
> 
> Even if they have the same semantics now, that might change in the
> future; restoring the original behaviour looks like the right thing to
> do.

I agree. Especially since we do care to preserve the behavior in
the !data branch. If there is no difference between the two, then it
would probably make sense to clean that up globally. 

Regards,
Halil

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ