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:   Tue, 17 Nov 2020 17:44:05 +0100
From:   Arnaud POULIQUEN <arnaud.pouliquen@...com>
To:     Guennadi Liakhovetski <guennadi.liakhovetski@...ux.intel.com>
CC:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        "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



On 11/17/20 5:03 PM, Guennadi Liakhovetski wrote:
> On Tue, Nov 17, 2020 at 12:42:30PM +0100, Arnaud POULIQUEN wrote:
> 
> [snip]
> 
>> 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);
> 
> Is this code requesting the module in which it is located?.. I must be missing 
> something...

Right this is stupid...Thanks to highlight this!

That being said, your remark is very interesting: we need to load the module to
access to this function. This means that calling this function ensures that the
module is loaded. In this case no need to add the piece of code to find
module... here is the call stack associated (associated patch is available below):


(rpmsg_ns_probe+0x5c/0xe0 [rpmsg_ns])
[   11.858748] [<bf00a0a0>] (rpmsg_ns_probe [rpmsg_ns]) from [<bf0005cc>]
(rpmsg_dev_probe+0x14c/0x1b0 [rpmsg_core])
[   11.869047] [<bf0005cc>] (rpmsg_dev_probe [rpmsg_core]) from [<c067cd44>]
(really_probe+0x208/0x4f0)
[   11.878117] [<c067cd44>] (really_probe) from [<c067d1f4>]
(driver_probe_device+0x78/0x16c)
[   11.886404] [<c067d1f4>] (driver_probe_device) from [<c067ad48>]
(bus_for_each_drv+0x84/0xd0)
[   11.894887] [<c067ad48>] (bus_for_each_drv) from [<c067ca9c>]
(__device_attach+0xf0/0x188)
[   11.903142] [<c067ca9c>] (__device_attach) from [<c067bb10>]
(bus_probe_device+0x84/0x8c)
[   11.911314] [<c067bb10>] (bus_probe_device) from [<c0678094>]
(device_add+0x3b0/0x7b0)
[   11.919227] [<c0678094>] (device_add) from [<bf0003dc>]
(rpmsg_register_device+0x54/0x88 [rpmsg_core])
[   11.928541] [<bf0003dc>] (rpmsg_register_device [rpmsg_core]) from
[<bf011b58>] (rpmsg_probe+0x298/0x3c8 [virtio_rpmsg_bus])
[   11.939748] [<bf011b58>] (rpmsg_probe [virtio_rpmsg_bus]) from [<c05cd648>]
(virtio_dev_probe+0x1f4/0x2c4)
[   11.949377] [<c05cd648>] (virtio_dev_probe) from [<c067cd44>]
(really_probe+0x208/0x4f0)
[   11.957454] [<c067cd44>] (really_probe) from [<c067d1f4>]
(driver_probe_device+0x78/0x16c)
[   11.965710] [<c067d1f4>] (driver_probe_device) from [<c067d548>]
(device_driver_attach+0x58/0x60)
[   11.974574] [<c067d548>] (device_driver_attach) from [<c067d604>]
(__driver_attach+0xb4/0x154)
[   11.983177] [<c067d604>] (__driver_attach) from [<c067ac68>]
(bus_for_each_dev+0x78/0xc0)
[   11.991344] [<c067ac68>] (bus_for_each_dev) from [<c067bdc0>]
(bus_add_driver+0x170/0x20c)
[   11.999600] [<c067bdc0>] (bus_add_driver) from [<c067e12c>]
(driver_register+0x74/0x108)
[   12.007693] [<c067e12c>] (driver_register) from [<bf017010>]
(rpmsg_init+0x10/0x1000 [virtio_rpmsg_bus])
[   12.017168] [<bf017010>] (rpmsg_init [virtio_rpmsg_bus]) from [<c0102090>]
(do_one_initcall+0x58/0x2bc)
[

This would make the patch very simple. I tested following patch on my platform,
applying it, i do not reproduce the initial issue.


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..5867281188de 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -55,6 +55,24 @@ 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)
+{
+	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 +98,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 +122,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

Thanks,
Arnaud

> 
> Thanks
> Guennadi
> 

Powered by blists - more mailing lists