| 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
| ||
|
Message-ID: <50549519-d9ff-9048-a3d8-dab02bfda096@st.com>
Date: Tue, 17 Nov 2020 12:42:30 +0100
From: Arnaud POULIQUEN <arnaud.pouliquen@...com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
CC: Guennadi Liakhovetski <guennadi.liakhovetski@...ux.intel.com>,
"ohad@...ery.com" <ohad@...ery.com>,
"bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 8/8] rpmsg: Turn name service into a stand alone driver
Hi Mathieu,
On 11/16/20 11:40 PM, Mathieu Poirier wrote:
> On Mon, Nov 16, 2020 at 04:51:52PM +0100, Arnaud POULIQUEN wrote:
>> Hi Guennadi,
>>
>> On 11/16/20 4:10 PM, Guennadi Liakhovetski wrote:
[snip]
> I did a lot of probing, went deep in the bowels of the user mode helper
> subsystem and looked at sys_load_module(). Especially at do_init_module() where
> function do_one_initcall()[1] is called on mod->init, which happens to be
> rpmsg_ns_init() where the name space driver is registered. I am confident we
> can rely on this mechanism.
>
> More comments below...
>
> [1]. https://elixir.bootlin.com/linux/v5.10-rc3/source/kernel/module.c#L3604
>
>> So if we can't conclude, adding completion would be OK for me.
>>
>> Thanks,
>> Arnaud
>>
[snip]
>
> We came up with almost exactly the same thing:>
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 5bda7cb44618..ab86b603c54e 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -81,6 +81,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
>
> static struct rpmsg_driver rpmsg_ns_driver = {
> .drv.name = "rpmsg_ns",
> + .drv.mod_name = "rpmsg_ns",
This does not work for me, in built-in the kernel freezes on start
i just have the " Starting kernel ..." print
Are you sure that is useful? it working for me without this.
> .probe = rpmsg_ns_probe,
> };
>
> @@ -104,5 +105,5 @@ module_exit(rpmsg_ns_exit);
>
> MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
> MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@...com>");
> -MODULE_ALIAS("rpmsg_ns");
> +MODULE_ALIAS("rpmsg:rpmsg_ns");
> MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
> index e267dd5f909b..28251fd1b2e0 100644
> --- a/include/linux/rpmsg/ns.h
> +++ b/include/linux/rpmsg/ns.h
> @@ -3,6 +3,7 @@
> #ifndef _LINUX_RPMSG_NS_H
> #define _LINUX_RPMSG_NS_H
>
> +#include <linux/module.h>
> #include <linux/mod_devicetable.h>
> #include <linux/rpmsg.h>
> #include <linux/rpmsg/byteorder.h>
> @@ -49,11 +50,27 @@ enum rpmsg_ns_flags {
> */
> static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
Agree with the Guennadi's pertinent remarks, better to put this in rpmsg_ns.c
> {
> + int ret;
> + struct module *rpmsg_ns;
> + const char name[] = "rpmsg_ns";
> +
> strcpy(rpdev->id.name, "rpmsg_ns");
> rpdev->driver_override = "rpmsg_ns";
> rpdev->src = RPMSG_NS_ADDR;
> rpdev->dst = RPMSG_NS_ADDR;
>
> +#ifdef CONFIG_MODULES
This piece of code is executed also if rppmsg_ns is built-in
> + mutex_lock(&module_mutex);
> + rpmsg_ns = find_module(name);
> + mutex_unlock(&module_mutex);
> +
> + if (!rpmsg_ns) {
> + ret = request_module(name);
> + if (ret)
> + pr_err("Can not find module %s (err %d)\n", name, ret);
As consequence for built-in this error is printed.
To avoid this
- #ifdef CONFIG_MODULES
+ #ifdef MODULES
Then for me here we should return the error.
> + }
> +#endif
> +
>
> I think it is better to be in rpmsg_ns_register_devices() because it makes the
> solution stand by itself, i.e no need to call the registration code from another
> driver. Everything is self contained.
That's a very good idea!
In addition I would keep dependency between virtio and rpmsg_ns in kconfig. This
would ensure that rpmsg ns is built for the virtio bus. Then the device will be
probed on demand.
>
> Also note the drv.mod_name = "rpmsg_ns" part. I took a look at find_module()
> and that is what is supposed to be used.
>
> That works on my side - please test on your setup.
Please find update of your patch integrating my remarks:
- suppress drv.mod_name,
- migrate rpmsg_ns_register_device in rpmsg_ns.c,
- use KBUILD_MODNAME for the module name,
- add select RPMSG_NS for RPMSG_VIRTIO config.
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index c3fc75e6514b..1394114782d2 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -71,5 +71,6 @@ config RPMSG_VIRTIO
depends on HAS_DMA
select RPMSG
select VIRTIO
+ select RPMSG_NS
endmenu
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 5bda7cb44618..80c2cc23bada 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,39 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void
*data, int len,
return 0;
}
+/**
+ * rpmsg_ns_register_device() - register name service device based on rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * basis for the rpmsg name service device.
+ */
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
+{
+#ifdef MODULES
+ int ret;
+ struct module *rpmsg_ns;
+
+ mutex_lock(&module_mutex);
+ rpmsg_ns = find_module(KBUILD_MODNAME);
+ mutex_unlock(&module_mutex);
+
+ if (!rpmsg_ns) {
+ ret = request_module(KBUILD_MODNAME);
+ if (ret)
+ pr_err("Can not find module %s (err %d)\n", KBUILD_MODNAME, ret);
+ }
+#endif
+
+ strcpy(rpdev->id.name, KBUILD_MODNAME);
+ rpdev->driver_override = KBUILD_MODNAME;
+ rpdev->src = RPMSG_NS_ADDR;
+ rpdev->dst = RPMSG_NS_ADDR;
+
+ return rpmsg_register_device(rpdev);
+}
+EXPORT_SYMBOL(rpmsg_ns_register_device);
+
static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
{
struct rpmsg_endpoint *ns_ept;
@@ -80,7 +113,7 @@ static int rpmsg_ns_probe(struct rpmsg_device *rpdev)
}
static struct rpmsg_driver rpmsg_ns_driver = {
- .drv.name = "rpmsg_ns",
+ .drv.name = KBUILD_MODNAME,
.probe = rpmsg_ns_probe,
};
@@ -104,5 +137,5 @@ module_exit(rpmsg_ns_exit);
MODULE_DESCRIPTION("Name service announcement rpmsg Driver");
MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@...com>");
-MODULE_ALIAS("rpmsg_ns");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h
index bdc1ea278814..68eac2b42075 100644
--- a/include/linux/rpmsg/ns.h
+++ b/include/linux/rpmsg/ns.h
@@ -41,21 +41,6 @@ enum rpmsg_ns_flags {
/* Address 53 is reserved for advertising remote services */
#define RPMSG_NS_ADDR (53)
-/**
- * rpmsg_ns_register_device() - register name service device based on rpdev
- * @rpdev: prepared rpdev to be used for creating endpoints
- *
- * This function wraps rpmsg_register_device() preparing the rpdev for use as
- * basis for the rpmsg name service device.
- */
-static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
-{
- strcpy(rpdev->id.name, "rpmsg_ns");
- rpdev->driver_override = "rpmsg_ns";
- rpdev->src = RPMSG_NS_ADDR;
- rpdev->dst = RPMSG_NS_ADDR;
-
- return rpmsg_register_device(rpdev);
-}
+int rpmsg_ns_register_device(struct rpmsg_device *rpdev);
#endif
Regards,
Arnaud
Powered by blists - more mailing lists