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

[Cornelia Huck]

On Fri, 13 Oct 2017 13:38:49 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> 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?

> 
> Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>
> ---
>  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.

> +}
> +
> +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.

> +		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?

> +
> +	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);