[PATCH v2 1/2] UBI: Fix section mismatch

[PATCH v2 1/2] UBI: Fix section mismatch

[Andy Shevchenko]

WARNING: vmlinux.o(.text+0x1f2a80): Section mismatch in reference from the variable __param_ops_mtd to the function .init.text:ubi_mtd_param_parse()
The function __param_ops_mtd() references
the function __init ubi_mtd_param_parse().
This is often because __param_ops_mtd lacks a __init
annotation or the annotation of ubi_mtd_param_parse is wrong.

Cc: Richard Weinberger <richard@nod.at>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mtd/ubi/build.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 85d54f37e28f..54312431750b 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -74,10 +74,10 @@ struct mtd_dev_param {
 };
 
 /* Numbers of elements set in the @mtd_dev_param array */
-static int __initdata mtd_devs;
+static int mtd_devs;
 
 /* MTD devices specification parameters */
-static struct mtd_dev_param __initdata mtd_dev_param[UBI_MAX_DEVICES];
+static struct mtd_dev_param mtd_dev_param[UBI_MAX_DEVICES];
 #ifdef CONFIG_MTD_UBI_FASTMAP
 /* UBI module parameter to enable fastmap automatically on non-fastmap images */
 static bool fm_autoconvert;
@@ -1351,7 +1351,7 @@ module_exit(ubi_exit);
  * This function returns positive resulting integer in case of success and a
  * negative error code in case of failure.
  */
-static int __init bytes_str_to_int(const char *str)
+static int bytes_str_to_int(const char *str)
 {
 	char *endp;
 	unsigned long result;
@@ -1389,7 +1389,7 @@ static int __init bytes_str_to_int(const char *str)
  * This function returns zero in case of success and a negative error code in
  * case of error.
  */
-static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
+static int ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
 {
 	int i, len;
 	struct mtd_dev_param *p;
-- 
2.11.0

[PATCH v2 2/2] UBI: Make mtd parameter readable

[Andy Shevchenko]

Fix permissions to allow read mtd parameter back (only for owner).

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mtd/ubi/build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 54312431750b..7bac790d4298 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1470,7 +1470,7 @@ static int ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
 	return 0;
 }
 
-module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
+module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
 MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024[,ubi_num]]].\n"
 		      "Multiple \"mtd\" parameters may be specified.\n"
 		      "MTD devices may be specified by their number, name, or path to the MTD character device node.\n"
-- 
2.11.0

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Richard Weinberger]

Andy,

On Tue, Jan 10, 2017 at 1:56 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Fix permissions to allow read mtd parameter back (only for owner).
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Please CC all maintainers in future. Otherwise the chance is high that
I'll miss a patch.

> ---
>  drivers/mtd/ubi/build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 54312431750b..7bac790d4298 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1470,7 +1470,7 @@ static int ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
>         return 0;
>  }
>
> -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
>  MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024[,ubi_num]]].\n"
>                       "Multiple \"mtd\" parameters may be specified.\n"
>                       "MTD devices may be specified by their number, name, or path to the MTD character device node.\n"

What is the use case?
AFAIKT the permissions are 000 because a parser is involved and to
"understand" the parameter,
a reader needs the ubi_mtd_param_parse() function.

-- 
Thanks,
//richard

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Andy Shevchenko]

On Tue, 2017-01-10 at 14:33 +0100, Richard Weinberger wrote:
> Andy,
> 
> On Tue, Jan 10, 2017 at 1:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Fix permissions to allow read mtd parameter back (only for owner).
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Please CC all maintainers in future. Otherwise the chance is high that
> I'll miss a patch.

Noted.

> > -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);

> What is the use case?
> AFAIKT the permissions are 000

If it's not 0 in current case than you easily crash the kernel because
parser will be gone at that time. This is fixed by patch 1.

>  because a parser is involved and to
> "understand" the parameter,
> a reader needs the ubi_mtd_param_parse() function.

Are you implying that writer is a bot and reader is human being? 
The use case is obvious (any security reasons are implied?) -- allow
user to see what was written there in the first place.

Permissions 0000 are error prone.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Richard Weinberger]

Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:
>>> -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
>>> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
> 
>> What is the use case?
>> AFAIKT the permissions are 000
> 
> If it's not 0 in current case than you easily crash the kernel because
> parser will be gone at that time. This is fixed by patch 1.

Before your changes it was non-issue, right? ;)

> 
>>  because a parser is involved and to
>> "understand" the parameter,
>> a reader needs the ubi_mtd_param_parse() function.
> 
> Are you implying that writer is a bot and reader is human being? 
> The use case is obvious (any security reasons are implied?) -- allow
> user to see what was written there in the first place.

I'm asking for the use case, why is exposing this parameter to user space
a good thing? Who will use it?

> Permissions 0000 are error prone.

Why?

Thanks,
//richard

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Andy Shevchenko]

On Tue, 2017-01-10 at 14:54 +0100, Richard Weinberger wrote:
> Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:
> > > > -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> > > > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
> > > What is the use case?
> > > AFAIKT the permissions are 000
> > 
> > If it's not 0 in current case than you easily crash the kernel
> > because
> > parser will be gone at that time. This is fixed by patch 1.
> 
> Before your changes it was non-issue, right? ;)

If annoying section mismatch is not an issue, then yes, correct.

> > >  because a parser is involved and to
> > > "understand" the parameter,
> > > a reader needs the ubi_mtd_param_parse() function.
> > 
> > Are you implying that writer is a bot and reader is human being? 
> > The use case is obvious (any security reasons are implied?) -- allow
> > user to see what was written there in the first place.
> 
> I'm asking for the use case, why is exposing this parameter to user
> space
> a good thing? Who will use it?

Any user. I like the idea to have initial string to be stored somewhere
and visible (imagine the case when it's module and history is gone). It
might be useful for debugging (okay, this case perhaps not anymore, but
in general).

> > Permissions 0000 are error prone.
> 
> Why?

Because of a trick is being used here.

P.S. If you strongly against it I will give up, it doesn't cost my
efforts anymore. That's why one of the possible "solution" is to put
comment there for other brave guys.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Richard Weinberger]

Am 10.01.2017 um 15:11 schrieb Andy Shevchenko:
> On Tue, 2017-01-10 at 14:54 +0100, Richard Weinberger wrote:
>> Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:
>>>>> -module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
>>>>> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 0400);
>>>> What is the use case?
>>>> AFAIKT the permissions are 000
>>>
>>> If it's not 0 in current case than you easily crash the kernel
>>> because
>>> parser will be gone at that time. This is fixed by patch 1.
>>
>> Before your changes it was non-issue, right? ;)
> 
> If annoying section mismatch is not an issue, then yes, correct.

Fixing the section mismatch is a good thing.

>>>>  because a parser is involved and to
>>>> "understand" the parameter,
>>>> a reader needs the ubi_mtd_param_parse() function.
>>>
>>> Are you implying that writer is a bot and reader is human being? 
>>> The use case is obvious (any security reasons are implied?) -- allow
>>> user to see what was written there in the first place.
>>
>> I'm asking for the use case, why is exposing this parameter to user
>> space
>> a good thing? Who will use it?
> 
> Any user. I like the idea to have initial string to be stored somewhere
> and visible (imagine the case when it's module and history is gone). It
> might be useful for debugging (okay, this case perhaps not anymore, but
> in general).
> 
>>> Permissions 0000 are error prone.
>>
>> Why?
> 
> Because of a trick is being used here.

You are not really the answering questions kind of guy? ;-)

> P.S. If you strongly against it I will give up, it doesn't cost my
> efforts anymore. That's why one of the possible "solution" is to put
> comment there for other brave guys.

I'm not at all against it, all I want to know why your patch improves the current
situation what it fixes.

Did you check? Do other drivers in the kernel that have a parser behind a non-trivial
module parameter also expose it to user space?

Thanks,
//richard

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Andy Shevchenko]

On Tue, 2017-01-10 at 15:16 +0100, Richard Weinberger wrote:
> Am 10.01.2017 um 15:11 schrieb Andy Shevchenko:
> > On Tue, 2017-01-10 at 14:54 +0100, Richard Weinberger wrote:
> > > Am 10.01.2017 um 14:48 schrieb Andy Shevchenko:

> > > > Permissions 0000 are error prone.
> > > 
> > > Why?
> > 
> > Because of a trick is being used here.
> 
> You are not really the answering questions kind of guy? ;-)

You see, you have to keep in mind that you are not allowed to use
anything except 0 because parser is gone, variable is gone. I'm talking
about module_param_cb(..., 0) cases only.

> > P.S. If you strongly against it I will give up, it doesn't cost my
> > efforts anymore. That's why one of the possible "solution" is to put
> > comment there for other brave guys.

> Did you check? Do other drivers in the kernel that have a parser
> behind a non-trivial
> module parameter also expose it to user space?

Just checked. The only one which is not, this one.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Richard Weinberger]

Andy,

Am 10.01.2017 um 15:35 schrieb Andy Shevchenko:
>> Did you check? Do other drivers in the kernel that have a parser
>> behind a non-trivial
>> module parameter also expose it to user space?
> 
> Just checked. The only one which is not, this one.
> 

That's an excellent reason. :-)
But I see also:
mtd/devices/phram.c:module_param_call(phram, phram_param_call, NULL, NULL, 000);

If these two are the only users of 000, let's rip them out to be consistent
with the rest of the kernel.

Thanks,
//richard

Re: [PATCH v2 2/2] UBI: Make mtd parameter readable

[Andy Shevchenko]

On Tue, 2017-01-10 at 16:31 +0100, Richard Weinberger wrote:
> Andy,
> 
> Am 10.01.2017 um 15:35 schrieb Andy Shevchenko:
> > > Did you check? Do other drivers in the kernel that have a parser
> > > behind a non-trivial
> > > module parameter also expose it to user space?
> > 
> > Just checked. The only one which is not, this one.
> > 
> 
> That's an excellent reason. :-)
> But I see also:
> mtd/devices/phram.c:module_param_call(phram, phram_param_call, NULL,
> NULL, 000);
> 
> If these two are the only users of 000, let's rip them out to be
> consistent
> with the rest of the kernel.

Okay, this is full(?) list of those. 3 drivers.

$ git grep -n 'module_param_call(.*, 0\+)' drivers/
drivers/ide/ide.c:277:module_param_call(chs, ide_set_disk_chs, NULL,
NULL, 0);
drivers/ide/ide.c:351:module_param_call(ignore_cable,
ide_set_ignore_cable, NULL, NULL, 0);
drivers/mtd/devices/phram.c:301:module_param_call(phram,
phram_param_call, NULL, NULL, 000);
drivers/platform/x86/thinkpad_acpi.c:9666:      module_param_call(featur
e, set_ibm_param, NULL, NULL, 0); \

$ git grep -n 'module_param_cb(.*, 0\+)' drivers/
drivers/mtd/ubi/block.c:168:module_param_cb(block, &ubiblock_param_ops,
NULL, 0);

I doubt about worth to fix IDE (last time I sent clean up maintainer
told me not to do).

For thinkpad I will look at.

For MTD let's do it now.

So, would you like me to resend with / send separate fix to the other
one?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy