Message-ID: <VI1PR0402MB2800404173FCC18BB8604A89E0D80@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Date:   Mon, 19 Nov 2018 10:33:02 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     Laurentiu Tudor <laurentiu.tudor@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev-owner@...r.kernel.org" <netdev-owner@...r.kernel.org>,
        Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>,
        Horia Geanta <horia.geanta@....com>,
        Leo Li <leoyang.li@....com>
Subject: RE: [PATCH v2 2/4] bus: fsl-mc: add fsl-mc userspace support


> > Adding userspace support for the MC (Management Complex) means
> > exporting an ioctl capable device file representing the root resource container.
> >
> > This new functionality in the fsl-mc bus driver intends to provide
> > userspace applications an interface to interact with the MC firmware.
> >
> > Commands that are composed in userspace are sent to the MC firmware
> > through the FSL_MC_SEND_MC_COMMAND ioctl.  By default the implicit MC
> > I/O portal is used for this operation, but if the implicit one is
> > busy, a dynamic portal is allocated and then freed upon execution.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> 
> Overall, this looks almost sane :)
> 
> But I do have some minor comments below on the code after only a quick
> review.
> 
> 
> > ---
> > Changes in v2:
> >    - use root dprc MC portal by default
> >    - create the uapi misc device from the root dprc probe function
> >
> >  Documentation/ioctl/ioctl-number.txt |   1 +
> >  drivers/bus/fsl-mc/Kconfig           |   7 ++
> >  drivers/bus/fsl-mc/Makefile          |   3 +
> >  drivers/bus/fsl-mc/dprc-driver.c     |  14 ++-
> >  drivers/bus/fsl-mc/fsl-mc-private.h  |  41 ++++++++
> >  drivers/bus/fsl-mc/fsl-mc-uapi.c     | 179
> +++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/fsl_mc.h          |   9 ++
> >  7 files changed, 253 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/bus/fsl-mc/fsl-mc-uapi.c
> >
> > diff --git a/Documentation/ioctl/ioctl-number.txt
> > b/Documentation/ioctl/ioctl-number.txt
> > index af6f6ba..eaae7bf 100644
> > --- a/Documentation/ioctl/ioctl-number.txt
> > +++ b/Documentation/ioctl/ioctl-number.txt
> > @@ -171,6 +171,7 @@ Code  Seq#(hex)	Include File
> 	Comments
> >  'R'	00-1F	linux/random.h		conflict!
> >  'R'	01	linux/rfkill.h		conflict!
> >  'R'	C0-DF	net/bluetooth/rfcomm.h
> > +'R'	E0	uapi/linux/fsl_mc.h
> >  'S'	all	linux/cdrom.h		conflict!
> >  'S'	80-81	scsi/scsi_ioctl.h	conflict!
> >  'S'	82-FF	scsi/scsi.h		conflict!
> > diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
> > index c23c77c..cde6f40 100644
> > --- a/drivers/bus/fsl-mc/Kconfig
> > +++ b/drivers/bus/fsl-mc/Kconfig
> > @@ -14,3 +14,10 @@ config FSL_MC_BUS
> >  	  architecture.  The fsl-mc bus driver handles discovery of
> >  	  DPAA2 objects (which are represented as Linux devices) and
> >  	  binding objects to drivers.
> > +
> > +config FSL_MC_UAPI_SUPPORT
> > +	bool "Management Complex (MC) userspace support"
> > +	depends on FSL_MC_BUS
> > +	help
> > +	  Provides userspace support for creating/destroying/configuring
> > +	  DPAA2 objects in the Management Complex.
> > diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> > index 3c518c7..4ae292a 100644
> > --- a/drivers/bus/fsl-mc/Makefile
> > +++ b/drivers/bus/fsl-mc/Makefile
> > @@ -16,3 +16,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
> >  		      fsl-mc-allocator.o \
> >  		      fsl-mc-msi.o \
> >  		      dpmcp.o
> > +
> > +# MC userspace support
> > +obj-$(CONFIG_FSL_MC_UAPI_SUPPORT) += fsl-mc-uapi.o
> > diff --git a/drivers/bus/fsl-mc/dprc-driver.c
> > b/drivers/bus/fsl-mc/dprc-driver.c
> > index 52c7e15..3919d9b 100644
> > --- a/drivers/bus/fsl-mc/dprc-driver.c
> > +++ b/drivers/bus/fsl-mc/dprc-driver.c
> > @@ -593,6 +593,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_dev);
> >  	bool mc_io_created = false;
> >  	bool msi_domain_set = false;
> > +	bool uapi_created = false;
> 
> I dont understand this "created" need/use.  Why is that needed?

The misc device should be created only for the root dprc device probed by this driver. The uapi_created flag is used to keep track if we are trying to probe the root dprc and if we succeeded in creating its misc device. Also, this is used on the cleanup path. If there are any other errors afterwards, we should know if the misc device is to be deregistered or not.

> 
> >  	u16 major_ver, minor_ver;
> >
> >  	if (!is_fsl_mc_bus_dprc(mc_dev))
> > @@ -647,6 +648,11 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  		} else {
> >  			dev_set_msi_domain(&mc_dev->dev,
> mc_msi_domain);
> >  			msi_domain_set = true;
> > +
> > +			error = fsl_mc_uapi_create_device_file(mc_bus);
> > +			if (error < 0)
> > +				goto error_cleanup_msi_domain;
> > +			uapi_created = true;
> 
> Why are you setting this, can't you always just "know" if the misc file was
> created so when you clean up from errors you can remove it?  Or is the logic
> here just too complex?
> 

We end up here if the dprc-driver is probing a root device. If this is the case, the misc_register called from fsl_mc_uapi_create_device_file could still fail if the misc driver is not yet probed. If this is the case, we just want to delay the probe.

This being said, I can just check for the root dprc and cleanup its misc device on the error path (in the error_cleanup_uapi label) without the need of the created flag.

> 
> >  		}
> >  	}
> >
> > @@ -654,7 +660,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  			  &mc_dev->mc_handle);
> >  	if (error < 0) {
> >  		dev_err(&mc_dev->dev, "dprc_open() failed: %d\n", error);
> > -		goto error_cleanup_msi_domain;
> > +		goto error_cleanup_uapi;
> >  	}
> >
> >  	error = dprc_get_attributes(mc_dev->mc_io, 0, mc_dev->mc_handle,
> @@
> > -706,6 +712,10 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  error_cleanup_open:
> >  	(void)dprc_close(mc_dev->mc_io, 0, mc_dev->mc_handle);
> >
> > +error_cleanup_uapi:
> > +	if (uapi_created)
> > +		fsl_mc_uapi_remove_device_file(mc_bus);
> > +
> >  error_cleanup_msi_domain:
> >  	if (msi_domain_set)
> >  		dev_set_msi_domain(&mc_dev->dev, NULL); @@ -774,6
> +784,8 @@ static
> > int dprc_remove(struct fsl_mc_device *mc_dev)
> >  	if (!fsl_mc_is_root_dprc(&mc_dev->dev)) {
> >  		fsl_destroy_mc_io(mc_dev->mc_io);
> >  		mc_dev->mc_io = NULL;
> > +	} else {
> > +		fsl_mc_uapi_remove_device_file(mc_bus);
> 
> You aren't checkig that bool flag here, which kind of implies that you didn't need
> it above as well.

In the _probe function, the uapi_created flag has somewhat the same meaning as the fsl_mc_is_root_dprc() here. I will change the probe function to use it also.

> 
> >  	}
> >
> >  	dev_info(&mc_dev->dev, "DPRC device unbound from driver"); diff
> > --git a/drivers/bus/fsl-mc/fsl-mc-private.h
> > b/drivers/bus/fsl-mc/fsl-mc-private.h
> > index ea11b4f..16902f9 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-private.h
> > +++ b/drivers/bus/fsl-mc/fsl-mc-private.h
> > @@ -10,6 +10,8 @@
> >
> >  #include <linux/fsl/mc.h>
> >  #include <linux/mutex.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/miscdevice.h>
> >
> >  /*
> >   * Data Path Management Complex (DPMNG) General API @@ -492,6 +494,24
> > @@ struct fsl_mc_resource_pool {  };
> >
> >  /**
> > + * struct fsl_mc_uapi - information associated with a device file
> > + * @misc: struct miscdevice linked to the root dprc
> > + * @device: newly created device in /dev
> > + * @mutex: mutex lock to serialize the open/release operations
> > + * @local_instance_in_use: local MC I/O instance in use or not
> > + * @dynamic_instance_count: number of dynamically created MC I/O
> > +instances
> > + * @static_mc_io: pointer to the static MC I/O object  */ struct
> > +fsl_mc_uapi {
> > +	struct miscdevice misc;
> > +	struct device *device;
> > +	struct mutex mutex; /* serialize open/release operations */
> > +	u32 local_instance_in_use;
> 
> Why do you care/need to know if it is in use or not?
> Why does it matter, that's a userspace decision to make, not the kernel's.

If the userspace is trying to make concurrent calls to the MC, those should be made using different MC portals.
A default/local portal is used by default so we keep track if this is in use or not so that we can make the next decision, whether we need to dynamically allocate another one when we encounter concurrent calls. 
I do not see how this is a userspace decision to make.

> 
> 
> > +	u32 dynamic_instance_count;
> > +	struct fsl_mc_io *static_mc_io;
> > +};
> > +
> > +/**
> >   * struct fsl_mc_bus - logical bus that corresponds to a physical DPRC
> >   * @mc_dev: fsl-mc device for the bus device itself.
> >   * @resource_pools: array of resource pools (one pool per resource
> > type) @@ -500,6 +520,7 @@ struct fsl_mc_resource_pool {
> >   * @irq_resources: Pointer to array of IRQ objects for the IRQ pool
> >   * @scan_mutex: Serializes bus scanning
> >   * @dprc_attr: DPRC attributes
> > + * @uapi_misc: struct that abstracts the interaction with userspace
> >   */
> >  struct fsl_mc_bus {
> >  	struct fsl_mc_device mc_dev;
> > @@ -507,6 +528,7 @@ struct fsl_mc_bus {
> >  	struct fsl_mc_device_irq *irq_resources;
> >  	struct mutex scan_mutex;    /* serializes bus scanning */
> >  	struct dprc_attributes dprc_attr;
> > +	struct fsl_mc_uapi uapi_misc;
> >  };
> >
> >  #define to_fsl_mc_bus(_mc_dev) \
> > @@ -561,4 +583,23 @@ int __must_check fsl_create_mc_io(struct device
> > *dev,
> >
> >  bool fsl_mc_is_root_dprc(struct device *dev);
> >
> > +#ifdef CONFIG_FSL_MC_UAPI_SUPPORT
> > +
> > +int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus);
> > +
> > +void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus);
> > +
> > +#else
> > +
> > +static inline int fsl_mc_uapi_create_device_file(struct fsl_mc_bus
> > +*mc_bus) {
> > +	return 0;
> > +}
> > +
> > +static inline void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus
> > +*mc_bus) { }
> > +
> > +#endif
> > +
> >  #endif /* _FSL_MC_PRIVATE_H_ */
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-uapi.c
> > b/drivers/bus/fsl-mc/fsl-mc-uapi.c
> > new file mode 100644
> > index 0000000..8c9debb
> > --- /dev/null
> > +++ b/drivers/bus/fsl-mc/fsl-mc-uapi.c
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Management Complex (MC) userspace support
> > + *
> > + * Copyright 2018 NXP
> > + *
> > + */
> > +
> > +#include <linux/slab.h>
> > +#include <linux/fs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/miscdevice.h>
> > +
> > +#include "fsl-mc-private.h"
> > +
> > +struct uapi_priv_data {
> > +	struct fsl_mc_uapi *uapi;
> > +	struct fsl_mc_io *mc_io;
> > +};
> > +
> > +static int fsl_mc_uapi_send_command(unsigned long arg,
> > +				    struct fsl_mc_io *mc_io)
> > +{
> > +	struct fsl_mc_command mc_cmd;
> > +	int error;
> > +
> > +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> > +	if (error)
> > +		return -EFAULT;
> > +
> > +	error = mc_send_command(mc_io, &mc_cmd);
> 
> Is there no need for any type of validation of the command at all?
> Previously you were always sending "known good" commands from within the
> kernel.  THat is suddenly changed here, how good and robust is your error
> handling in this function?

The only measure taken here is the fact that we only accept data of fsl_mc_command size.
This is because we are relying on the MC firmware, which already has validation logic, to parse the commands and return an error in case the command is malformed or with invalid parameters
Also, doubling the command validation code inside the kernel would create unnecessary complexity.

> 
> 
> > +	if (error)
> > +		return error;
> > +
> > +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > +	if (error)
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_mc_uapi_dev_open(struct inode *inode, struct file
> > +*filep) {
> > +	struct fsl_mc_device *root_mc_device;
> > +	struct uapi_priv_data *priv_data;
> > +	struct fsl_mc_io *dynamic_mc_io;
> > +	struct fsl_mc_uapi *mc_uapi;
> > +	struct fsl_mc_bus *mc_bus;
> > +	int error;
> > +
> > +	priv_data = kzalloc(sizeof(*priv_data), GFP_KERNEL);
> > +	if (!priv_data)
> > +		return -ENOMEM;
> > +
> > +	mc_uapi = container_of(filep->private_data, struct fsl_mc_uapi, misc);
> > +	mc_bus = container_of(mc_uapi, struct fsl_mc_bus, uapi_misc);
> > +	root_mc_device = &mc_bus->mc_dev;
> > +
> > +	mutex_lock(&mc_uapi->mutex);
> > +
> > +	if (!mc_uapi->local_instance_in_use) {
> > +		priv_data->mc_io = mc_uapi->static_mc_io;
> > +		mc_uapi->local_instance_in_use = true;
> 
> You are setting a u32 to "true"?  The compiler did not complain?
> 

Sorry for this, will fix. The compiler did not complain.. maybe I should upgrade.

> And again, why is this needed, I don't understand the distinction.

I tried to answer this question above. We should know if the default MC portal is in use so that we can allocate a new one if this the case.

> 
> > +	} else {
> > +		error = fsl_mc_portal_allocate(root_mc_device, 0,
> > +					       &dynamic_mc_io);
> > +		if (error) {
> > +			pr_err("Could not allocate MC portal\n");
> > +			goto error_portal_allocate;
> > +		}
> > +
> > +		mc_uapi->dynamic_instance_count++;
> 
> Why care about the "count"?

We can certainly live without it. It was mainly used as a debug counter to check of everything is freed properly.

> 
> 
> > +		priv_data->mc_io = dynamic_mc_io;
> > +	}
> > +	priv_data->uapi = mc_uapi;
> > +	filep->private_data = priv_data;
> > +
> > +	mutex_unlock(&mc_uapi->mutex);
> > +
> > +	return 0;
> > +
> > +error_portal_allocate:
> > +	mutex_unlock(&mc_uapi->mutex);
> > +
> > +	return error;
> > +}
> > +
> > +static int fsl_mc_uapi_dev_release(struct inode *inode, struct file
> > +*filep) {
> > +	struct uapi_priv_data *priv_data;
> > +	struct fsl_mc_uapi *mc_uapi;
> > +	struct fsl_mc_io *mc_io;
> > +
> > +	priv_data = filep->private_data;
> > +	mc_uapi = priv_data->uapi;
> > +	mc_io = priv_data->mc_io;
> > +
> > +	mutex_lock(&mc_uapi->mutex);
> > +
> > +	if (WARN_ON(!mc_uapi->local_instance_in_use &&
> > +		    mc_uapi->dynamic_instance_count == 0)) {
> > +		mutex_unlock(&mc_uapi->mutex);
> > +		return -EINVAL;
> 
> Never have a WARN_ON() on a path that userspace can generate, otherwise
> when you run with panic_on_warn enabled, you just crashed the machine.
> 
> If this really is such a horrible error, do a "dev_warn()" and move on.
> But again, do not print something if userspace can trigger it all the time, that's a
> local DoS.

The entire WARN_ON can be removed. This could never happen.

> 
> 
> > +	}
> > +
> > +	if (mc_io == mc_uapi->static_mc_io) {
> > +		mc_uapi->local_instance_in_use = false;
> > +	} else {
> > +		fsl_mc_portal_free(mc_io);
> > +		mc_uapi->dynamic_instance_count--;
> > +	}
> > +
> > +	kfree(filep->private_data);
> > +	filep->private_data =  NULL;
> > +
> > +	mutex_unlock(&mc_uapi->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static long fsl_mc_uapi_dev_ioctl(struct file *file,
> > +				  unsigned int cmd,
> > +				  unsigned long arg)
> > +{
> > +	struct uapi_priv_data *priv_data = file->private_data;
> > +	int error;
> > +
> > +	switch (cmd) {
> > +	case FSL_MC_SEND_MC_COMMAND:
> > +		error = fsl_mc_uapi_send_command(arg, priv_data->mc_io);
> > +		break;
> > +	default:
> > +		pr_err("%s: unexpected ioctl call number\n", __func__);
> > +		error = -EINVAL;
> 
> dev_err()?  Same for all of your pr_* calls, use the dev_* calls instead.
> 
> And again, this is something that userspace can easily trigger, do not spam the
> logs with this type of message.  Make it dev_dbg() instead for your own
> debugging use.
> 

Will do.

> 
> > +	}
> > +
> > +	return error;
> > +}
> > +
> > +static const struct file_operations fsl_mc_uapi_dev_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = fsl_mc_uapi_dev_open,
> > +	.release = fsl_mc_uapi_dev_release,
> > +	.unlocked_ioctl = fsl_mc_uapi_dev_ioctl, };
> > +
> > +int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus) {
> > +	struct fsl_mc_device *mc_dev = &mc_bus->mc_dev;
> > +	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
> > +	int error;
> > +
> > +	mc_uapi->misc.minor = MISC_DYNAMIC_MINOR;
> > +	mc_uapi->misc.name = dev_name(&mc_dev->dev);
> > +	mc_uapi->misc.fops = &fsl_mc_uapi_dev_fops;
> > +
> > +	error = misc_register(&mc_uapi->misc);
> > +	if (error)
> > +		return -EPROBE_DEFER;
> 
> No, return the error given to you, don't eat it and make up a new one.

Ok. So the caller should make the decision to defer or not?

> 
> > +
> > +	mc_uapi->static_mc_io = mc_bus->mc_dev.mc_io;
> > +
> > +	mutex_init(&mc_uapi->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus) {
> > +	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
> > +
> > +	if (WARN_ON(mc_uapi->local_instance_in_use))
> > +		return;
> > +
> > +	if (WARN_ON(mc_uapi->dynamic_instance_count != 0))
> > +		return;
> 
> Same comments as above on the WARN_ON(), don't do it.  make it
> dev_dbg() if you really just want to make sure your code is correct.  If it can
> happen "in the wild", then you need to refactor the code to prevent this from
> ever happening (hint, can it ever happen?  I don't think so but I might be
> wrong...)

Yep, these are more paranoid checks and cannot ever happen. I will just remove them. 

Will come back with a v3 to include the needed changes.

Thanks,
Ioana C

> 
> thanks,
> 
> greg k-h

Powered by blists - more mailing lists