[PATCH 0/4] rpmsg: ctrl: Add ability to instantiate rpmsg device locally

[PATCH 0/4] rpmsg: ctrl: Add ability to instantiate rpmsg device locally

[Arnaud Pouliquen]

Purpose:
  Allow the user space application to create and release an rpmsg device by adding
  rpmsg ioctrl to the /dev/rpmsg_ctrl interface
    
Aim:
  The current implementation is based on the enumeration of services by the 
  remote processor to create a new channel and instantiate associated rpmsg device.
  There is no solution to create a rpmsg channel on user application request.
  If the rpmsg char driver allows adding a new endpoint over an existing channel, it
  does not offer the ability to create a new one.
  Adding the IOCTRL to instantiate rpmsg channels from the user application will allow
  to dynamically create and destroy rpmsg devices. Some examples of use are:
  - activate the service at the initiative of the application,
  - remove the communication on a specific channel before entering the suspend mode,
  - creating a temporary channel for debugging purposes.

Concerns:
  This implementation is very simple but allows the user application to create rpmsg devices
  without any limitations.
  - A device can be created even if there is no match with the driver (and it is difficult to
    check a match with module drivers).
  - It is not really possible to add a counter to limit the number of devices, because a device
    can be released by the remote side.
  
  The question is: should we add protection? Notice that there is no protection for RPMSG_CREATE_EPT_IOCTL.

  If this is not considered safe, another approach could be to create an rpmsg_ctrl API to add controls.
  This API would be used by a rpmsg driver to add is own user controls, such as creating/deleting devices.

How to test it:
  - This series can be applied on git/andersson/remoteproc.git for-next branch (dc0e14fa833b)
    + the "Restructure the rpmsg char to decorrelate the control part" series[1]
  - to test the ioctrl, a rpmsgexportdev tool is available here: https://github.com/arnopo/rpmsgexport

[1]https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793

Arnaud Pouliquen (4):
  rpmsg: ctrl: Introduce RPMSG_CREATE_DEV_IOCTL
  rpmsg: ctrl: Introduce RPMSG_RELEASE_DEV_IOCTL
  rpmsg: ctrl: Add check on rpmsg device removability from user space
  rpmsg: Add a removable attribute to the rpmsg device

 drivers/rpmsg/rpmsg_core.c |  2 ++
 drivers/rpmsg/rpmsg_ctrl.c | 52 +++++++++++++++++++++++++++++++++++---
 include/linux/rpmsg.h      |  2 ++
 include/uapi/linux/rpmsg.h | 10 ++++++++
 4 files changed, 62 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 1/4] rpmsg: ctrl: Introduce RPMSG_CREATE_DEV_IOCTL

[Arnaud Pouliquen]

Implement the RPMSG_CREATE_DEV_IOCTL to allow the user application to
initiate a communication through a new rpmsg channel.
This Ioctl can be used to instantiate a local rpmsg device.
Depending on the back-end implementation, the associated rpmsg driver is
probed and a NS announcement can be sent to the remote processor.

Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 30 ++++++++++++++++++++++++++----
 include/uapi/linux/rpmsg.h |  5 +++++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index eeb1708548c1..4aa962df3661 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -23,6 +23,7 @@
 #include <uapi/linux/rpmsg.h>
 
 #include "rpmsg_char.h"
+#include "rpmsg_internal.h"
 
 static dev_t rpmsg_major;
 
@@ -37,11 +38,13 @@ static DEFINE_IDA(rpmsg_minor_ida);
  * @rpdev:	underlaying rpmsg device
  * @cdev:	cdev for the ctrl device
  * @dev:	device for the ctrl device
+ * @ctrl_lock:	serialize the ioctrls.
  */
 struct rpmsg_ctrldev {
 	struct rpmsg_device *rpdev;
 	struct cdev cdev;
 	struct device dev;
+	struct mutex ctrl_lock;
 };
 
 static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
@@ -70,9 +73,8 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	struct rpmsg_endpoint_info eptinfo;
 	struct rpmsg_channel_info chinfo;
-
-	if (cmd != RPMSG_CREATE_EPT_IOCTL)
-		return -EINVAL;
+	struct rpmsg_device *rpdev;
+	int ret = 0;
 
 	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
 		return -EFAULT;
@@ -82,7 +84,26 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	chinfo.src = eptinfo.src;
 	chinfo.dst = eptinfo.dst;
 
-	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
+	mutex_lock(&ctrldev->ctrl_lock);
+	switch (cmd) {
+	case RPMSG_CREATE_EPT_IOCTL:
+		ret = rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
+		break;
+
+	case RPMSG_CREATE_DEV_IOCTL:
+		rpdev = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+		if (!rpdev) {
+			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
+			ret = -ENXIO;
+		}
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&ctrldev->ctrl_lock);
+
+	return ret;
 };
 
 static const struct file_operations rpmsg_ctrldev_fops = {
@@ -119,6 +140,7 @@ static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
 	device_initialize(dev);
 	dev->parent = &rpdev->dev;
 
+	mutex_init(&ctrldev->ctrl_lock);
 	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
 	ctrldev->cdev.owner = THIS_MODULE;
 
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f5ca8740f3fb..f9d5a74e7801 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -33,4 +33,9 @@ struct rpmsg_endpoint_info {
  */
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
+/**
+ * Instantiate a rpmsg service device.
+ */
+#define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
+
 #endif
-- 
2.17.1

[PATCH 2/4] rpmsg: ctrl: Introduce RPMSG_RELEASE_DEV_IOCTL

[Arnaud Pouliquen]

Implement the RPMSG_RELEASE_DEV_IOCTL to allow the user application to
release a rpmsg device created either by the remote processor or with
the RPMSG_CREATE_DEV_IOCTL call.
Depending on the back-end implementation, the associated rpmsg driver is
removed and a NS destroy rpmsg can be sent to the remote processor.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 7 +++++++
 include/uapi/linux/rpmsg.h | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 4aa962df3661..cb19e32d05e1 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -98,6 +98,13 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 		}
 		break;
 
+	case RPMSG_RELEASE_DEV_IOCTL:
+		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
+		if (ret)
+			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
+				chinfo.name, ret);
+		break;
+
 	default:
 		ret = -EINVAL;
 	}
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f9d5a74e7801..38639ba37438 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -38,4 +38,9 @@ struct rpmsg_endpoint_info {
  */
 #define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
 
+/**
+ * Release a local rpmsg device.
+ */
+#define RPMSG_RELEASE_DEV_IOCTL	_IOW(0xb5, 0x4, struct rpmsg_endpoint_info)
+
 #endif
-- 
2.17.1

[PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Arnaud Pouliquen]

Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).

Add a new field to store the removability of the device.

By default the rpmsg device can not be removed by user space. It is
set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
could also be set by an rpmsg driver during probe.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
 include/linux/rpmsg.h      |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index cb19e32d05e1..e93c6ec49038 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	struct rpmsg_endpoint_info eptinfo;
 	struct rpmsg_channel_info chinfo;
 	struct rpmsg_device *rpdev;
+	struct device *dev;
 	int ret = 0;
 
 	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
@@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 		if (!rpdev) {
 			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
 			ret = -ENXIO;
+		} else {
+			/* Allow user space to release the device. */
+			rpdev->us_removable = 1;
 		}
 		break;
 
 	case RPMSG_RELEASE_DEV_IOCTL:
-		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
+		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
+		if (!dev)
+			ret =  -ENXIO;
+
+		/* Verify that rpmsg device removal is allowed. */
+		if (!ret) {
+			rpdev = to_rpmsg_device(dev);
+			if (!rpdev->us_removable)
+				ret = -EACCES;
+		}
+		if (!ret)
+			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
 		if (ret)
 			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
 				chinfo.name, ret);
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index d97dcd049f18..3642aad1a789 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -47,6 +47,7 @@ struct rpmsg_channel_info {
  * @ept: the rpmsg endpoint of this channel
  * @announce: if set, rpmsg will announce the creation/removal of this channel
  * @little_endian: True if transport is using little endian byte representation
+ * @us_removable: True if userspace application has permission to remove the rpmsg device
  */
 struct rpmsg_device {
 	struct device dev;
@@ -57,6 +58,7 @@ struct rpmsg_device {
 	struct rpmsg_endpoint *ept;
 	bool announce;
 	bool little_endian;
+	bool us_removable;
 
 	const struct rpmsg_device_ops *ops;
 };
-- 
2.17.1

[PATCH 4/4] rpmsg: Add a removable attribute to the rpmsg device

[Arnaud Pouliquen]

Adds a new attribute to the rpmsg device to expose in sysfs the
the removability of an rpmsg device.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index e5daee4f9373..b2543ef4a92f 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -413,6 +413,7 @@ rpmsg_show_attr(src, src, "0x%x\n");
 rpmsg_show_attr(dst, dst, "0x%x\n");
 rpmsg_show_attr(announce, announce ? "true" : "false", "%s\n");
 rpmsg_string_attr(driver_override, driver_override);
+rpmsg_show_attr(removable, us_removable ? "true" : "false", "%s\n");
 
 static ssize_t modalias_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
@@ -435,6 +436,7 @@ static struct attribute *rpmsg_dev_attrs[] = {
 	&dev_attr_src.attr,
 	&dev_attr_announce.attr,
 	&dev_attr_driver_override.attr,
+	&dev_attr_removable.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(rpmsg_dev);
-- 
2.17.1

Re: [PATCH 2/4] rpmsg: ctrl: Introduce RPMSG_RELEASE_DEV_IOCTL

[Mathieu Poirier]

Good day,

On Fri, Jun 04, 2021 at 11:14:04AM +0200, Arnaud Pouliquen wrote:
> Implement the RPMSG_RELEASE_DEV_IOCTL to allow the user application to
> release a rpmsg device created either by the remote processor or with
> the RPMSG_CREATE_DEV_IOCTL call.
> Depending on the back-end implementation, the associated rpmsg driver is
> removed and a NS destroy rpmsg can be sent to the remote processor.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 7 +++++++
>  include/uapi/linux/rpmsg.h | 5 +++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 4aa962df3661..cb19e32d05e1 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -98,6 +98,13 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  		}
>  		break;
>  
> +	case RPMSG_RELEASE_DEV_IOCTL:
> +		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> +		if (ret)
> +			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
> +				chinfo.name, ret);
> +		break;
> +

Please move the content of this patch in 1/4.  

>  	default:
>  		ret = -EINVAL;
>  	}
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index f9d5a74e7801..38639ba37438 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -38,4 +38,9 @@ struct rpmsg_endpoint_info {
>   */
>  #define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
>  
> +/**
> + * Release a local rpmsg device.
> + */
> +#define RPMSG_RELEASE_DEV_IOCTL	_IOW(0xb5, 0x4, struct rpmsg_endpoint_info)
> +
>  #endif
> -- 
> 2.17.1
> 

Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Mathieu Poirier]

On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
> 
> Add a new field to store the removability of the device.
> 
> By default the rpmsg device can not be removed by user space. It is
> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
> could also be set by an rpmsg driver during probe.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>  include/linux/rpmsg.h      |  2 ++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index cb19e32d05e1..e93c6ec49038 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  	struct rpmsg_endpoint_info eptinfo;
>  	struct rpmsg_channel_info chinfo;
>  	struct rpmsg_device *rpdev;
> +	struct device *dev;
>  	int ret = 0;
>  
>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  		if (!rpdev) {
>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>  			ret = -ENXIO;
> +		} else {
> +			/* Allow user space to release the device. */
> +			rpdev->us_removable = 1;

As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
can attain the same result by looking at chinfo->name, chinfo->src and
chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
operation is refused.

That way we don't introduce a new flag and there is also no need to call
rpmsg_find_device() twice.

Thanks,
Mathieu

>  		}
>  		break;
>  
>  	case RPMSG_RELEASE_DEV_IOCTL:
> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
> +		if (!dev)
> +			ret =  -ENXIO;
> +
> +		/* Verify that rpmsg device removal is allowed. */
> +		if (!ret) {
> +			rpdev = to_rpmsg_device(dev);
> +			if (!rpdev->us_removable)
> +				ret = -EACCES;
> +		}
> +		if (!ret)
> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>  		if (ret)
>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>  				chinfo.name, ret);
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index d97dcd049f18..3642aad1a789 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>   * @ept: the rpmsg endpoint of this channel
>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>   * @little_endian: True if transport is using little endian byte representation
> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>   */
>  struct rpmsg_device {
>  	struct device dev;
> @@ -57,6 +58,7 @@ struct rpmsg_device {
>  	struct rpmsg_endpoint *ept;
>  	bool announce;
>  	bool little_endian;
> +	bool us_removable;
>  
>  	const struct rpmsg_device_ops *ops;
>  };
> -- 
> 2.17.1
> 

Re: [PATCH 4/4] rpmsg: Add a removable attribute to the rpmsg device

[Mathieu Poirier]

On Fri, Jun 04, 2021 at 11:14:06AM +0200, Arnaud Pouliquen wrote:
> Adds a new attribute to the rpmsg device to expose in sysfs the
> the removability of an rpmsg device.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index e5daee4f9373..b2543ef4a92f 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -413,6 +413,7 @@ rpmsg_show_attr(src, src, "0x%x\n");
>  rpmsg_show_attr(dst, dst, "0x%x\n");
>  rpmsg_show_attr(announce, announce ? "true" : "false", "%s\n");
>  rpmsg_string_attr(driver_override, driver_override);
> +rpmsg_show_attr(removable, us_removable ? "true" : "false", "%s\n");
>  
>  static ssize_t modalias_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
> @@ -435,6 +436,7 @@ static struct attribute *rpmsg_dev_attrs[] = {
>  	&dev_attr_src.attr,
>  	&dev_attr_announce.attr,
>  	&dev_attr_driver_override.attr,
> +	&dev_attr_removable.attr,

And this patch shouldn't be needed if we move forward with my recommendation on
patch 3/4.

>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(rpmsg_dev);
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] rpmsg: ctrl: Introduce RPMSG_CREATE_DEV_IOCTL

[Mathieu Poirier]

On Fri, Jun 04, 2021 at 11:14:03AM +0200, Arnaud Pouliquen wrote:
> Implement the RPMSG_CREATE_DEV_IOCTL to allow the user application to
> initiate a communication through a new rpmsg channel.
> This Ioctl can be used to instantiate a local rpmsg device.
> Depending on the back-end implementation, the associated rpmsg driver is
> probed and a NS announcement can be sent to the remote processor.
> 
> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 30 ++++++++++++++++++++++++++----
>  include/uapi/linux/rpmsg.h |  5 +++++
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index eeb1708548c1..4aa962df3661 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -23,6 +23,7 @@
>  #include <uapi/linux/rpmsg.h>
>  
>  #include "rpmsg_char.h"
> +#include "rpmsg_internal.h"
>  
>  static dev_t rpmsg_major;
>  
> @@ -37,11 +38,13 @@ static DEFINE_IDA(rpmsg_minor_ida);
>   * @rpdev:	underlaying rpmsg device
>   * @cdev:	cdev for the ctrl device
>   * @dev:	device for the ctrl device
> + * @ctrl_lock:	serialize the ioctrls.
>   */
>  struct rpmsg_ctrldev {
>  	struct rpmsg_device *rpdev;
>  	struct cdev cdev;
>  	struct device dev;
> +	struct mutex ctrl_lock;
>  };
>  
>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> @@ -70,9 +73,8 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  	void __user *argp = (void __user *)arg;
>  	struct rpmsg_endpoint_info eptinfo;
>  	struct rpmsg_channel_info chinfo;
> -
> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> -		return -EINVAL;
> +	struct rpmsg_device *rpdev;
> +	int ret = 0;
>  
>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>  		return -EFAULT;
> @@ -82,7 +84,26 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  	chinfo.src = eptinfo.src;
>  	chinfo.dst = eptinfo.dst;
>  
> -	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
> +	mutex_lock(&ctrldev->ctrl_lock);

Do we need a lock here?  I thought the character device layer would guarantee
accesses on a file handler would be atomic...  Am I wrong?

> +	switch (cmd) {
> +	case RPMSG_CREATE_EPT_IOCTL:
> +		ret = rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
> +		break;
> +
> +	case RPMSG_CREATE_DEV_IOCTL:
> +		rpdev = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> +		if (!rpdev) {
> +			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
> +			ret = -ENXIO;
> +		}
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&ctrldev->ctrl_lock);
> +
> +	return ret;
>  };
>  
>  static const struct file_operations rpmsg_ctrldev_fops = {
> @@ -119,6 +140,7 @@ static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
>  	device_initialize(dev);
>  	dev->parent = &rpdev->dev;
>  
> +	mutex_init(&ctrldev->ctrl_lock);
>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>  	ctrldev->cdev.owner = THIS_MODULE;
>  
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index f5ca8740f3fb..f9d5a74e7801 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -33,4 +33,9 @@ struct rpmsg_endpoint_info {
>   */
>  #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
>  
> +/**
> + * Instantiate a rpmsg service device.
> + */
> +#define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
> +
>  #endif
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] rpmsg: ctrl: Introduce RPMSG_CREATE_DEV_IOCTL

[Arnaud POULIQUEN]

Hello Mathieu,

On 6/15/21 7:53 PM, Mathieu Poirier wrote:
> On Fri, Jun 04, 2021 at 11:14:03AM +0200, Arnaud Pouliquen wrote:
>> Implement the RPMSG_CREATE_DEV_IOCTL to allow the user application to
>> initiate a communication through a new rpmsg channel.
>> This Ioctl can be used to instantiate a local rpmsg device.
>> Depending on the back-end implementation, the associated rpmsg driver is
>> probed and a NS announcement can be sent to the remote processor.
>>
>> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 30 ++++++++++++++++++++++++++----
>>  include/uapi/linux/rpmsg.h |  5 +++++
>>  2 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index eeb1708548c1..4aa962df3661 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -23,6 +23,7 @@
>>  #include <uapi/linux/rpmsg.h>
>>  
>>  #include "rpmsg_char.h"
>> +#include "rpmsg_internal.h"
>>  
>>  static dev_t rpmsg_major;
>>  
>> @@ -37,11 +38,13 @@ static DEFINE_IDA(rpmsg_minor_ida);
>>   * @rpdev:	underlaying rpmsg device
>>   * @cdev:	cdev for the ctrl device
>>   * @dev:	device for the ctrl device
>> + * @ctrl_lock:	serialize the ioctrls.
>>   */
>>  struct rpmsg_ctrldev {
>>  	struct rpmsg_device *rpdev;
>>  	struct cdev cdev;
>>  	struct device dev;
>> +	struct mutex ctrl_lock;
>>  };
>>  
>>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>> @@ -70,9 +73,8 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  	void __user *argp = (void __user *)arg;
>>  	struct rpmsg_endpoint_info eptinfo;
>>  	struct rpmsg_channel_info chinfo;
>> -
>> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> -		return -EINVAL;
>> +	struct rpmsg_device *rpdev;
>> +	int ret = 0;
>>  
>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>  		return -EFAULT;
>> @@ -82,7 +84,26 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  	chinfo.src = eptinfo.src;
>>  	chinfo.dst = eptinfo.dst;
>>  
>> -	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
>> +	mutex_lock(&ctrldev->ctrl_lock);
> 
> Do we need a lock here?  I thought the character device layer would guarantee
> accesses on a file handler would be atomic...  Am I wrong?
> 

It is a good point! from my understanding, using "unlocked_ioctl" ops, the
driver has to handle is own atomic protection.
I will try to hack the code to verify this.

Thanks,
Arnaud

>> +	switch (cmd) {
>> +	case RPMSG_CREATE_EPT_IOCTL:
>> +		ret = rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
>> +		break;
>> +
>> +	case RPMSG_CREATE_DEV_IOCTL:
>> +		rpdev = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>> +		if (!rpdev) {
>> +			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>> +			ret = -ENXIO;
>> +		}
>> +		break;
>> +
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +	mutex_unlock(&ctrldev->ctrl_lock);
>> +
>> +	return ret;
>>  };
>>  
>>  static const struct file_operations rpmsg_ctrldev_fops = {
>> @@ -119,6 +140,7 @@ static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
>>  	device_initialize(dev);
>>  	dev->parent = &rpdev->dev;
>>  
>> +	mutex_init(&ctrldev->ctrl_lock);
>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>  	ctrldev->cdev.owner = THIS_MODULE;
>>  
>> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
>> index f5ca8740f3fb..f9d5a74e7801 100644
>> --- a/include/uapi/linux/rpmsg.h
>> +++ b/include/uapi/linux/rpmsg.h
>> @@ -33,4 +33,9 @@ struct rpmsg_endpoint_info {
>>   */
>>  #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
>>  
>> +/**
>> + * Instantiate a rpmsg service device.
>> + */
>> +#define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
>> +
>>  #endif
>> -- 
>> 2.17.1
>>

Re: [PATCH 2/4] rpmsg: ctrl: Introduce RPMSG_RELEASE_DEV_IOCTL

[Arnaud POULIQUEN]

On 6/15/21 7:38 PM, Mathieu Poirier wrote:
> Good day,
> 
> On Fri, Jun 04, 2021 at 11:14:04AM +0200, Arnaud Pouliquen wrote:
>> Implement the RPMSG_RELEASE_DEV_IOCTL to allow the user application to
>> release a rpmsg device created either by the remote processor or with
>> the RPMSG_CREATE_DEV_IOCTL call.
>> Depending on the back-end implementation, the associated rpmsg driver is
>> removed and a NS destroy rpmsg can be sent to the remote processor.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 7 +++++++
>>  include/uapi/linux/rpmsg.h | 5 +++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index 4aa962df3661..cb19e32d05e1 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -98,6 +98,13 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  		}
>>  		break;
>>  
>> +	case RPMSG_RELEASE_DEV_IOCTL:
>> +		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>> +		if (ret)
>> +			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>> +				chinfo.name, ret);
>> +		break;
>> +
> 
> Please move the content of this patch in 1/4.  

ok

> 
>>  	default:
>>  		ret = -EINVAL;
>>  	}
>> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
>> index f9d5a74e7801..38639ba37438 100644
>> --- a/include/uapi/linux/rpmsg.h
>> +++ b/include/uapi/linux/rpmsg.h
>> @@ -38,4 +38,9 @@ struct rpmsg_endpoint_info {
>>   */
>>  #define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
>>  
>> +/**
>> + * Release a local rpmsg device.
>> + */
>> +#define RPMSG_RELEASE_DEV_IOCTL	_IOW(0xb5, 0x4, struct rpmsg_endpoint_info)
>> +
>>  #endif
>> -- 
>> 2.17.1
>>

Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Arnaud POULIQUEN]

On 6/15/21 7:46 PM, Mathieu Poirier wrote:
> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
>>
>> Add a new field to store the removability of the device.
>>
>> By default the rpmsg device can not be removed by user space. It is
>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
>> could also be set by an rpmsg driver during probe.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>>  include/linux/rpmsg.h      |  2 ++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index cb19e32d05e1..e93c6ec49038 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  	struct rpmsg_endpoint_info eptinfo;
>>  	struct rpmsg_channel_info chinfo;
>>  	struct rpmsg_device *rpdev;
>> +	struct device *dev;
>>  	int ret = 0;
>>  
>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  		if (!rpdev) {
>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>  			ret = -ENXIO;
>> +		} else {
>> +			/* Allow user space to release the device. */
>> +			rpdev->us_removable = 1;
> 
> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
> can attain the same result by looking at chinfo->name, chinfo->src and
> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
> operation is refused.

Something must have escaped me, because i turn around your your proposal,
without understand it.

The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
that have not to be released by user application. Either because there are core
( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
rpmsg devices.

look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
by the RPMSG_RELEASE_DEV_IOCTL.

Please, could you clarify what you have in mind here?

Thanks,
Arnaud

> 
> That way we don't introduce a new flag and there is also no need to call
> rpmsg_find_device() twice.



> 
> Thanks,
> Mathieu
> 
>>  		}
>>  		break;
>>  
>>  	case RPMSG_RELEASE_DEV_IOCTL:
>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
>> +		if (!dev)
>> +			ret =  -ENXIO;
>> +
>> +		/* Verify that rpmsg device removal is allowed. */
>> +		if (!ret) {
>> +			rpdev = to_rpmsg_device(dev);
>> +			if (!rpdev->us_removable)
>> +				ret = -EACCES;
>> +		}
>> +		if (!ret)
>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>  		if (ret)
>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>>  				chinfo.name, ret);
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>> index d97dcd049f18..3642aad1a789 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>>   * @ept: the rpmsg endpoint of this channel
>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>>   * @little_endian: True if transport is using little endian byte representation
>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>>   */
>>  struct rpmsg_device {
>>  	struct device dev;
>> @@ -57,6 +58,7 @@ struct rpmsg_device {
>>  	struct rpmsg_endpoint *ept;
>>  	bool announce;
>>  	bool little_endian;
>> +	bool us_removable;
>>  
>>  	const struct rpmsg_device_ops *ops;
>>  };
>> -- 
>> 2.17.1
>>

Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Mathieu Poirier]

On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
> 
> 
> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
> > On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
> >> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
> >> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
> >>
> >> Add a new field to store the removability of the device.
> >>
> >> By default the rpmsg device can not be removed by user space. It is
> >> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
> >> could also be set by an rpmsg driver during probe.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
> >>  include/linux/rpmsg.h      |  2 ++
> >>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >> index cb19e32d05e1..e93c6ec49038 100644
> >> --- a/drivers/rpmsg/rpmsg_ctrl.c
> >> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>  	struct rpmsg_endpoint_info eptinfo;
> >>  	struct rpmsg_channel_info chinfo;
> >>  	struct rpmsg_device *rpdev;
> >> +	struct device *dev;
> >>  	int ret = 0;
> >>  
> >>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>  		if (!rpdev) {
> >>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
> >>  			ret = -ENXIO;
> >> +		} else {
> >> +			/* Allow user space to release the device. */
> >> +			rpdev->us_removable = 1;
> > 
> > As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
> > can attain the same result by looking at chinfo->name, chinfo->src and
> > chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
> > something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
> > to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
> > operation is refused.
> 
> Something must have escaped me, because i turn around your your proposal,
> without understand it.
> 
> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
> that have not to be released by user application. Either because there are core
> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
> rpmsg devices.
>

I don't see how the current patch would allow a driver to prevent user space
from releasing a rpmsg device since the sysfs attribute can be changed at will.
So even if the driver sets the flag user space can still revert it.

> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
> by the RPMSG_RELEASE_DEV_IOCTL.

That is correct.  I did not address rpmsg_ns to keep things simple but it would
also have to be handled properly.

> 
> Please, could you clarify what you have in mind here?

Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
- if a user space process with root privileges can't be trusted then we have
bigger problems than unwanted releases of registered rpmsg devices.

> 
> Thanks,
> Arnaud
> 
> > 
> > That way we don't introduce a new flag and there is also no need to call
> > rpmsg_find_device() twice.
> 
> 
> 
> > 
> > Thanks,
> > Mathieu
> > 
> >>  		}
> >>  		break;
> >>  
> >>  	case RPMSG_RELEASE_DEV_IOCTL:
> >> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
> >> +		if (!dev)
> >> +			ret =  -ENXIO;
> >> +
> >> +		/* Verify that rpmsg device removal is allowed. */
> >> +		if (!ret) {
> >> +			rpdev = to_rpmsg_device(dev);
> >> +			if (!rpdev->us_removable)
> >> +				ret = -EACCES;
> >> +		}
> >> +		if (!ret)
> >> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >>  		if (ret)
> >>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
> >>  				chinfo.name, ret);
> >> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> >> index d97dcd049f18..3642aad1a789 100644
> >> --- a/include/linux/rpmsg.h
> >> +++ b/include/linux/rpmsg.h
> >> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
> >>   * @ept: the rpmsg endpoint of this channel
> >>   * @announce: if set, rpmsg will announce the creation/removal of this channel
> >>   * @little_endian: True if transport is using little endian byte representation
> >> + * @us_removable: True if userspace application has permission to remove the rpmsg device
> >>   */
> >>  struct rpmsg_device {
> >>  	struct device dev;
> >> @@ -57,6 +58,7 @@ struct rpmsg_device {
> >>  	struct rpmsg_endpoint *ept;
> >>  	bool announce;
> >>  	bool little_endian;
> >> +	bool us_removable;
> >>  
> >>  	const struct rpmsg_device_ops *ops;
> >>  };
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Arnaud POULIQUEN]

Hello Mathieu,

On 6/16/21 7:15 PM, Mathieu Poirier wrote:
> On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
>>> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
>>>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
>>>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
>>>>
>>>> Add a new field to store the removability of the device.
>>>>
>>>> By default the rpmsg device can not be removed by user space. It is
>>>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
>>>> could also be set by an rpmsg driver during probe.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>>>>  include/linux/rpmsg.h      |  2 ++
>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>> index cb19e32d05e1..e93c6ec49038 100644
>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>  	struct rpmsg_endpoint_info eptinfo;
>>>>  	struct rpmsg_channel_info chinfo;
>>>>  	struct rpmsg_device *rpdev;
>>>> +	struct device *dev;
>>>>  	int ret = 0;
>>>>  
>>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>  		if (!rpdev) {
>>>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>>>  			ret = -ENXIO;
>>>> +		} else {
>>>> +			/* Allow user space to release the device. */
>>>> +			rpdev->us_removable = 1;
>>>
>>> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
>>> can attain the same result by looking at chinfo->name, chinfo->src and
>>> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
>>> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
>>> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
>>> operation is refused.
>>
>> Something must have escaped me, because i turn around your your proposal,
>> without understand it.
>>
>> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
>> that have not to be released by user application. Either because there are core
>> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
>> rpmsg devices.
>>
> 
> I don't see how the current patch would allow a driver to prevent user space
> from releasing a rpmsg device since the sysfs attribute can be changed at will.
> So even if the driver sets the flag user space can still revert it.


The patch [4/4] define the a read only attribute using the rpmsg_show_attr
declaration[1]. So the userspace can't change it.

This also has the advantage of not allowing the new IOCTRL API to be used by
default for legacy RPMSg devices without a specific patch.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L362

> 
>> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
>> by the RPMSG_RELEASE_DEV_IOCTL.
> 
> That is correct.  I did not address rpmsg_ns to keep things simple but it would
> also have to be handled properly.
> 
>>
>> Please, could you clarify what you have in mind here?
> 
> Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
> mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
> - if a user space process with root privileges can't be trusted then we have
> bigger problems than unwanted releases of registered rpmsg devices.

That's make sense. If we go on this way we could also trust the root application
for the rpmsg_ns and only protect the rpmsg_ctrl which can not release itself,
as you proposed.

Thanks,

Arnaud

> 
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> That way we don't introduce a new flag and there is also no need to call
>>> rpmsg_find_device() twice.
>>
>>
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>>  		}
>>>>  		break;
>>>>  
>>>>  	case RPMSG_RELEASE_DEV_IOCTL:
>>>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
>>>> +		if (!dev)
>>>> +			ret =  -ENXIO;
>>>> +
>>>> +		/* Verify that rpmsg device removal is allowed. */
>>>> +		if (!ret) {
>>>> +			rpdev = to_rpmsg_device(dev);
>>>> +			if (!rpdev->us_removable)
>>>> +				ret = -EACCES;
>>>> +		}
>>>> +		if (!ret)
>>>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>>  		if (ret)
>>>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>>>>  				chinfo.name, ret);
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index d97dcd049f18..3642aad1a789 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>>>>   * @ept: the rpmsg endpoint of this channel
>>>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>>>>   * @little_endian: True if transport is using little endian byte representation
>>>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>>>>   */
>>>>  struct rpmsg_device {
>>>>  	struct device dev;
>>>> @@ -57,6 +58,7 @@ struct rpmsg_device {
>>>>  	struct rpmsg_endpoint *ept;
>>>>  	bool announce;
>>>>  	bool little_endian;
>>>> +	bool us_removable;
>>>>  
>>>>  	const struct rpmsg_device_ops *ops;
>>>>  };
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Arnaud POULIQUEN]

On 6/17/21 10:02 AM, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 6/16/21 7:15 PM, Mathieu Poirier wrote:
>> On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
>>>> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
>>>>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
>>>>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
>>>>>
>>>>> Add a new field to store the removability of the device.
>>>>>
>>>>> By default the rpmsg device can not be removed by user space. It is
>>>>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
>>>>> could also be set by an rpmsg driver during probe.
>>>>>
>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>>> ---
>>>>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>>>>>  include/linux/rpmsg.h      |  2 ++
>>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>>> index cb19e32d05e1..e93c6ec49038 100644
>>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>>  	struct rpmsg_endpoint_info eptinfo;
>>>>>  	struct rpmsg_channel_info chinfo;
>>>>>  	struct rpmsg_device *rpdev;
>>>>> +	struct device *dev;
>>>>>  	int ret = 0;
>>>>>  
>>>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>>  		if (!rpdev) {
>>>>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>>>>  			ret = -ENXIO;
>>>>> +		} else {
>>>>> +			/* Allow user space to release the device. */
>>>>> +			rpdev->us_removable = 1;
>>>>
>>>> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
>>>> can attain the same result by looking at chinfo->name, chinfo->src and
>>>> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
>>>> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
>>>> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
>>>> operation is refused.
>>>
>>> Something must have escaped me, because i turn around your your proposal,
>>> without understand it.
>>>
>>> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
>>> that have not to be released by user application. Either because there are core
>>> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
>>> rpmsg devices.
>>>
>>
>> I don't see how the current patch would allow a driver to prevent user space
>> from releasing a rpmsg device since the sysfs attribute can be changed at will.
>> So even if the driver sets the flag user space can still revert it.
> 
> 
> The patch [4/4] define the a read only attribute using the rpmsg_show_attr
> declaration[1]. So the userspace can't change it.
> 
> This also has the advantage of not allowing the new IOCTRL API to be used by
> default for legacy RPMSg devices without a specific patch.
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L362
> 
>>
>>> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
>>> by the RPMSG_RELEASE_DEV_IOCTL.
>>
>> That is correct.  I did not address rpmsg_ns to keep things simple but it would
>> also have to be handled properly.
>>
>>>
>>> Please, could you clarify what you have in mind here?
>>
>> Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
>> mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
>> - if a user space process with root privileges can't be trusted then we have
>> bigger problems than unwanted releases of registered rpmsg devices.
> 
> That's make sense. If we go on this way we could also trust the root application
> for the rpmsg_ns and only protect the rpmsg_ctrl which can not release itself,
> as you proposed.

As discussed in the OpenAMP by-weekly meeting, I will send a new revision,
without the attribute.

Thanks,
Arnaud

> 
> Thanks,
> 
> Arnaud
> 
>>
>>>
>>> Thanks,
>>> Arnaud
>>>
>>>>
>>>> That way we don't introduce a new flag and there is also no need to call
>>>> rpmsg_find_device() twice.
>>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>>  		}
>>>>>  		break;
>>>>>  
>>>>>  	case RPMSG_RELEASE_DEV_IOCTL:
>>>>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
>>>>> +		if (!dev)
>>>>> +			ret =  -ENXIO;
>>>>> +
>>>>> +		/* Verify that rpmsg device removal is allowed. */
>>>>> +		if (!ret) {
>>>>> +			rpdev = to_rpmsg_device(dev);
>>>>> +			if (!rpdev->us_removable)
>>>>> +				ret = -EACCES;
>>>>> +		}
>>>>> +		if (!ret)
>>>>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>>>  		if (ret)
>>>>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>>>>>  				chinfo.name, ret);
>>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>>> index d97dcd049f18..3642aad1a789 100644
>>>>> --- a/include/linux/rpmsg.h
>>>>> +++ b/include/linux/rpmsg.h
>>>>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>>>>>   * @ept: the rpmsg endpoint of this channel
>>>>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>>>>>   * @little_endian: True if transport is using little endian byte representation
>>>>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>>>>>   */
>>>>>  struct rpmsg_device {
>>>>>  	struct device dev;
>>>>> @@ -57,6 +58,7 @@ struct rpmsg_device {
>>>>>  	struct rpmsg_endpoint *ept;
>>>>>  	bool announce;
>>>>>  	bool little_endian;
>>>>> +	bool us_removable;
>>>>>  
>>>>>  	const struct rpmsg_device_ops *ops;
>>>>>  };
>>>>> -- 
>>>>> 2.17.1
>>>>>

Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

[Mathieu Poirier]

On Thu, Jun 17, 2021 at 10:02:14AM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 6/16/21 7:15 PM, Mathieu Poirier wrote:
> > On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
> >>
> >>
> >> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
> >>> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
> >>>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
> >>>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
> >>>>
> >>>> Add a new field to store the removability of the device.
> >>>>
> >>>> By default the rpmsg device can not be removed by user space. It is
> >>>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
> >>>> could also be set by an rpmsg driver during probe.
> >>>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>> ---
> >>>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
> >>>>  include/linux/rpmsg.h      |  2 ++
> >>>>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >>>> index cb19e32d05e1..e93c6ec49038 100644
> >>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
> >>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >>>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>>>  	struct rpmsg_endpoint_info eptinfo;
> >>>>  	struct rpmsg_channel_info chinfo;
> >>>>  	struct rpmsg_device *rpdev;
> >>>> +	struct device *dev;
> >>>>  	int ret = 0;
> >>>>  
> >>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >>>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>>>  		if (!rpdev) {
> >>>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
> >>>>  			ret = -ENXIO;
> >>>> +		} else {
> >>>> +			/* Allow user space to release the device. */
> >>>> +			rpdev->us_removable = 1;
> >>>
> >>> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
> >>> can attain the same result by looking at chinfo->name, chinfo->src and
> >>> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
> >>> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
> >>> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
> >>> operation is refused.
> >>
> >> Something must have escaped me, because i turn around your your proposal,
> >> without understand it.
> >>
> >> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
> >> that have not to be released by user application. Either because there are core
> >> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
> >> rpmsg devices.
> >>
> > 
> > I don't see how the current patch would allow a driver to prevent user space
> > from releasing a rpmsg device since the sysfs attribute can be changed at will.
> > So even if the driver sets the flag user space can still revert it.
> 
> 
> The patch [4/4] define the a read only attribute using the rpmsg_show_attr
> declaration[1]. So the userspace can't change it.
> 

You are correct - I overlooked the RO attribute in the rpmsg_show_attr() macro.

> This also has the advantage of not allowing the new IOCTRL API to be used by
> default for legacy RPMSg devices without a specific patch.
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L362
> 
> > 
> >> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
> >> by the RPMSG_RELEASE_DEV_IOCTL.
> > 
> > That is correct.  I did not address rpmsg_ns to keep things simple but it would
> > also have to be handled properly.
> > 
> >>
> >> Please, could you clarify what you have in mind here?
> > 
> > Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
> > mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
> > - if a user space process with root privileges can't be trusted then we have
> > bigger problems than unwanted releases of registered rpmsg devices.
> 
> That's make sense. If we go on this way we could also trust the root application
> for the rpmsg_ns and only protect the rpmsg_ctrl which can not release itself,
> as you proposed.

I think we should protect both of them or neither of them.  I'd be fine with either
solution.

> 
> Thanks,
> 
> Arnaud
> 
> > 
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>> That way we don't introduce a new flag and there is also no need to call
> >>> rpmsg_find_device() twice.
> >>
> >>
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>>  		}
> >>>>  		break;
> >>>>  
> >>>>  	case RPMSG_RELEASE_DEV_IOCTL:
> >>>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >>>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
> >>>> +		if (!dev)
> >>>> +			ret =  -ENXIO;
> >>>> +
> >>>> +		/* Verify that rpmsg device removal is allowed. */
> >>>> +		if (!ret) {
> >>>> +			rpdev = to_rpmsg_device(dev);
> >>>> +			if (!rpdev->us_removable)
> >>>> +				ret = -EACCES;
> >>>> +		}
> >>>> +		if (!ret)
> >>>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >>>>  		if (ret)
> >>>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
> >>>>  				chinfo.name, ret);
> >>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> >>>> index d97dcd049f18..3642aad1a789 100644
> >>>> --- a/include/linux/rpmsg.h
> >>>> +++ b/include/linux/rpmsg.h
> >>>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
> >>>>   * @ept: the rpmsg endpoint of this channel
> >>>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
> >>>>   * @little_endian: True if transport is using little endian byte representation
> >>>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
> >>>>   */
> >>>>  struct rpmsg_device {
> >>>>  	struct device dev;
> >>>> @@ -57,6 +58,7 @@ struct rpmsg_device {
> >>>>  	struct rpmsg_endpoint *ept;
> >>>>  	bool announce;
> >>>>  	bool little_endian;
> >>>> +	bool us_removable;
> >>>>  
> >>>>  	const struct rpmsg_device_ops *ops;
> >>>>  };
> >>>> -- 
> >>>> 2.17.1
> >>>>

Re: [PATCH 1/4] rpmsg: ctrl: Introduce RPMSG_CREATE_DEV_IOCTL

[Arnaud POULIQUEN]

On 6/16/21 10:12 AM, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 6/15/21 7:53 PM, Mathieu Poirier wrote:
>> On Fri, Jun 04, 2021 at 11:14:03AM +0200, Arnaud Pouliquen wrote:
>>> Implement the RPMSG_CREATE_DEV_IOCTL to allow the user application to
>>> initiate a communication through a new rpmsg channel.
>>> This Ioctl can be used to instantiate a local rpmsg device.
>>> Depending on the back-end implementation, the associated rpmsg driver is
>>> probed and a NS announcement can be sent to the remote processor.
>>>
>>> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>> ---
>>>  drivers/rpmsg/rpmsg_ctrl.c | 30 ++++++++++++++++++++++++++----
>>>  include/uapi/linux/rpmsg.h |  5 +++++
>>>  2 files changed, 31 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>> index eeb1708548c1..4aa962df3661 100644
>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>> @@ -23,6 +23,7 @@
>>>  #include <uapi/linux/rpmsg.h>
>>>  
>>>  #include "rpmsg_char.h"
>>> +#include "rpmsg_internal.h"
>>>  
>>>  static dev_t rpmsg_major;
>>>  
>>> @@ -37,11 +38,13 @@ static DEFINE_IDA(rpmsg_minor_ida);
>>>   * @rpdev:	underlaying rpmsg device
>>>   * @cdev:	cdev for the ctrl device
>>>   * @dev:	device for the ctrl device
>>> + * @ctrl_lock:	serialize the ioctrls.
>>>   */
>>>  struct rpmsg_ctrldev {
>>>  	struct rpmsg_device *rpdev;
>>>  	struct cdev cdev;
>>>  	struct device dev;
>>> +	struct mutex ctrl_lock;
>>>  };
>>>  
>>>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>>> @@ -70,9 +73,8 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>  	void __user *argp = (void __user *)arg;
>>>  	struct rpmsg_endpoint_info eptinfo;
>>>  	struct rpmsg_channel_info chinfo;
>>> -
>>> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>>> -		return -EINVAL;
>>> +	struct rpmsg_device *rpdev;
>>> +	int ret = 0;
>>>  
>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>  		return -EFAULT;
>>> @@ -82,7 +84,26 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>  	chinfo.src = eptinfo.src;
>>>  	chinfo.dst = eptinfo.dst;
>>>  
>>> -	return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
>>> +	mutex_lock(&ctrldev->ctrl_lock);
>>
>> Do we need a lock here?  I thought the character device layer would guarantee
>> accesses on a file handler would be atomic...  Am I wrong?
>>
> 
> It is a good point! from my understanding, using "unlocked_ioctl" ops, the
> driver has to handle is own atomic protection.
> I will try to hack the code to verify this.

I confirm without lock there is no atomic access, re-entrance is possible in
rpmsg_ctrldev_ioctl. Keeping lock to serialize the controls seems safer to me to
avoid race condition.

Regards
Arnaud

> 
> Thanks,
> Arnaud
> 
>>> +	switch (cmd) {
>>> +	case RPMSG_CREATE_EPT_IOCTL:
>>> +		ret = rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
>>> +		break;
>>> +
>>> +	case RPMSG_CREATE_DEV_IOCTL:
>>> +		rpdev = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>>> +		if (!rpdev) {
>>> +			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>> +			ret = -ENXIO;
>>> +		}
>>> +		break;
>>> +
>>> +	default:
>>> +		ret = -EINVAL;
>>> +	}
>>> +	mutex_unlock(&ctrldev->ctrl_lock);
>>> +
>>> +	return ret;
>>>  };
>>>  
>>>  static const struct file_operations rpmsg_ctrldev_fops = {
>>> @@ -119,6 +140,7 @@ static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
>>>  	device_initialize(dev);
>>>  	dev->parent = &rpdev->dev;
>>>  
>>> +	mutex_init(&ctrldev->ctrl_lock);
>>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>>  	ctrldev->cdev.owner = THIS_MODULE;
>>>  
>>> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
>>> index f5ca8740f3fb..f9d5a74e7801 100644
>>> --- a/include/uapi/linux/rpmsg.h
>>> +++ b/include/uapi/linux/rpmsg.h
>>> @@ -33,4 +33,9 @@ struct rpmsg_endpoint_info {
>>>   */
>>>  #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
>>>  
>>> +/**
>>> + * Instantiate a rpmsg service device.
>>> + */
>>> +#define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
>>> +
>>>  #endif
>>> -- 
>>> 2.17.1
>>>