| 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: <16e07968-d783-8bcc-cec1-fd02cd717ddd@st.com>
Date: Tue, 17 Nov 2020 18:30:54 +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:58 PM, Guennadi Liakhovetski wrote:
> On Tue, Nov 17, 2020 at 05:44:05PM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> 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):
>
> Yes, as I wrote 10 hours ago:
>
>> Now, as for how to actually load the
>> module, I'd really propose to move rpmsg_ns_register_device() into the .c
>> file and then the problem will be resolved automatically: as a symbol
>> dependence the module will be loaded whenever another module, calling
>> rpmsg_ns_register_device() is loaded.
It's not a good day for me today... it seems I read your explanation too quickly
this morning, which is, however, very clear.
My apologies
Arnaud
>
> Thanks
> Guennadi
>
>> (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