linux-kernel - Re: [RFC 04/19] s390/zcrypt: create an AP matrix device on the AP matrix bus

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]

Date:   Wed, 18 Oct 2017 13:54:34 -0400
From:   Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, freude@...ibm.com, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, borntraeger@...ibm.com,
        kwankhede@...dia.com, bjsdjshi@...ux.vnet.ibm.com,
        pbonzini@...hat.com, alex.williamson@...hat.com,
        pmorel@...ux.vnet.ibm.com, alifm@...ux.vnet.ibm.com,
        mjrosato@...ux.vnet.ibm.com, qemu-s390x@...gnu.org,
        jjherne@...ux.vnet.ibm.com, thuth@...hat.com,
        pasic@...ux.vnet.ibm.com
Subject: Re: [RFC 04/19] s390/zcrypt: create an AP matrix device on the AP
 matrix bus

On 10/18/2017 12:20 PM, Cornelia Huck wrote:
> On Fri, 13 Oct 2017 13:38:49 -0400
> Tony Krowiak <akrowiak@...ux.vnet.ibm.com> wrote:
>
> [Please take with a grain of salt as I did not yet have time to take
> more than a very superficial glance at the whole structure.]
>
>> Creates a single AP matrix device on the AP matrix bus.
>> The matrix device will be created as part of the AP matrix bus
>> initialization. The matrix device will hold the AP queues that
>> have been reserved for use by KVM guests. Mediated matrix devices
>> can then be created for each guest. The mediated matrix devices can
>> then be configured with a matrix of AP adapters, usage and
>> control domains that will be made accessible to the guest.
>>
>> The sysfs location of the matrix device is:
>>
>> /sys/bus/ap_matrix
>> ... [devices]
>> ...... [matrix]
> Also /sys/devices/ap_matrix/matrix, isn't it?
Yes
>
>> Signed-off-by: Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
>> ---
>>   drivers/s390/crypto/ap_matrix_bus.c |   54 +++++++++++++++++++++++++++++++++++
>>   drivers/s390/crypto/ap_matrix_bus.h |    6 ++++
>>   2 files changed, 60 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/ap_matrix_bus.c b/drivers/s390/crypto/ap_matrix_bus.c
>> index fbae175..4eb1e3c 100644
>> --- a/drivers/s390/crypto/ap_matrix_bus.c
>> +++ b/drivers/s390/crypto/ap_matrix_bus.c
>> @@ -12,6 +12,7 @@
>>   
>>   #include <linux/module.h>
>>   #include <linux/device.h>
>> +#include <linux/slab.h>
>>   #include <asm/ap.h>
>>   
>>   #include "ap_matrix_bus.h"
>> @@ -21,13 +22,59 @@
>>   MODULE_LICENSE("GPL v2");
>>   
>>   #define AP_MATRIX_BUS_NAME "ap_matrix"
>> +#define AP_MATRIX_DEV_TYPE_NAME "ap_matrix"
>> +#define AP_MATRIX_DEV_NAME "matrix"
>>   
>>   static struct device *ap_matrix_root_device;
>> +static struct ap_matrix *matrix;
>>   
>>   static struct bus_type ap_matrix_bus_type = {
>>   	.name = AP_MATRIX_BUS_NAME,
>>   };
>>   
>> +static struct device_type ap_matrix_type = {
>> +	.name = AP_MATRIX_DEV_TYPE_NAME,
>> +};
>> +
>> +static void ap_matrix_dev_release(struct device *dev)
>> +{
>> +	struct ap_matrix *ap_matrix;
>> +
>> +	ap_matrix = container_of(dev, struct ap_matrix, device);
>> +
>> +	if (matrix == ap_matrix)
>> +		kfree(matrix);
>> +
>> +	matrix = NULL;
> This looks very, very odd. Refcounting should take care that the
> release function is only invoked if the device is really gone.
>
> Also, I don't think you need to keep a pointer to the device as it is a
> singleton: It's simply the only device on the list and you should be
> able to easily pick it that way. If your code does not register further
> devices on the bus, there won't be ambiguities.
Okay, I'll make that change
>
>> +}
>> +
>> +static int ap_matrix_dev_create(void)
>> +{
>> +	int ret;
>> +
>> +	matrix = kzalloc(sizeof(*matrix), GFP_KERNEL);
>> +	if (!matrix)
>> +		return -ENOMEM;
>> +
>> +	matrix->device.type = &ap_matrix_type;
>> +	dev_set_name(&matrix->device, "%s", AP_MATRIX_DEV_NAME);
>> +	matrix->device.bus = &ap_matrix_bus_type;
>> +	matrix->device.parent = ap_matrix_root_device;
>> +	matrix->device.release = ap_matrix_dev_release;
>> +
>> +	ret = device_register(&matrix->device);
>> +	if (ret) {
>> +		put_device(&matrix->device);
>> +		kfree(matrix);
>> +		matrix = NULL;
> That's wrong. The release function needs to clean up the embedding
> structure, so that kfree is at best useless and at worst wrong, if
> something else grabbed a reference.
Good point.
>
>> +		return ret;
>> +	}
>> +
>> +	get_device(&matrix->device);
> Why should you need an extra reference here? I'd expect the code
> hanging devices off this one to properly grab a reference, so you
> should be all good?
I'll remove this.
>
>> +
>> +	return 0;
>> +}
>> +
>>   int __init ap_matrix_init(void)
>>   {
>>   	int ret;
>> @@ -41,8 +88,15 @@ int __init ap_matrix_init(void)
>>   	if (ret)
>>   		goto bus_reg_err;
>>   
>> +	ret = ap_matrix_dev_create();
>> +	if (ret)
>> +		goto matrix_create_err;
>> +
>>   	return 0;
>>   
>> +matrix_create_err:
>> +	bus_unregister(&ap_matrix_bus_type);
>> +
>>   bus_reg_err:
>>   	root_device_unregister(ap_matrix_root_device);