Patchwork [02/02] force module loaded with partitions set

login
register
mail settings
Submitter Yang Ruirui
Date March 14, 2011, 1:51 a.m.
Message ID <20110314015148.GA6827@darkstar>
Download mbox | patch
Permalink /patch/86655/
State New
Headers show

Comments

Yang Ruirui - March 14, 2011, 1:51 a.m.
From: Yang Ruirui<ruirui.r.yang@tieto.com>

partitions can not be set after module loaded, the moduel param mode is 0444.

this patch force module loaded with param partitions set, if user does not
set partitions then give out a warning and return -EINVAL

Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
Tested-by: Shao Yanqing<yanqing.shao@tieto.com>
Tested-by: Xiao Yang<yang.xiao@tieto.com>
---
 drivers/mtd/mtdswap.c |    6 ++++++
 1 file changed, 6 insertions(+)
Artem Bityutskiy - March 14, 2011, 8:43 a.m.
On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote:
> From: Yang Ruirui<ruirui.r.yang@tieto.com>
> 
> partitions can not be set after module loaded, the moduel param mode is 0444.
> 
> this patch force module loaded with param partitions set, if user does not
> set partitions then give out a warning and return -EINVAL
> 
> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
> Tested-by: Shao Yanqing<yanqing.shao@tieto.com>
> Tested-by: Xiao Yang<yang.xiao@tieto.com>
> ---
>  drivers/mtd/mtdswap.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c	2011-03-14 09:36:09.283329099 +0800
> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c	2011-03-14 09:46:30.229993534 +0800
> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o
>  
>  static int __init mtdswap_modinit(void)
>  {
> +	if (!partitions[0]) {
> +		printk(KERN_WARNING
> +			"Please load mtdswap with correct partitions param\n");
> +		return -EINVAL;
> +	}

I think a similar check is done in mtdswap_add_mtd() ?
Artem Bityutskiy - March 14, 2011, 8:53 a.m.
On Mon, 2011-03-14 at 16:57 +0800, Yang Rui Rui wrote:
> On 03/14/2011 04:43 PM, Artem Bityutskiy wrote:
> > On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote:
> >> From: Yang Ruirui<ruirui.r.yang@tieto.com>
> >>
> >> partitions can not be set after module loaded, the moduel param
> mode is 0444.
> >>
> >> this patch force module loaded with param partitions set, if user
> does not
> >> set partitions then give out a warning and return -EINVAL
> >>
> >> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
> >> Tested-by: Shao Yanqing<yanqing.shao@tieto.com>
> >> Tested-by: Xiao Yang<yang.xiao@tieto.com>
> >> ---
> >>   drivers/mtd/mtdswap.c |    6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c	2011-03-14
> 09:36:09.283329099 +0800
> >> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c	2011-03-14
> 09:46:30.229993534 +0800
> >> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o
> >>
> >>   static int __init mtdswap_modinit(void)
> >>   {
> >> +	if (!partitions[0]) {
> >> +		printk(KERN_WARNING
> >> +			"Please load mtdswap with correct partitions param\n");
> >> +		return -EINVAL;
> >> +	}
> >
> > I think a similar check is done in mtdswap_add_mtd() ?
> >
> >
> 
> Yes, that one should be removed if this is ok. This module just waste
> memory without partitions set. And there's no chance to pass in the
> params.

I agree that the problem you are solving exist. But I think I do not
agree with the solution. There are many other ways to end up with
mtdswap module loaded but not functioning, e.g., users passes incorrect
module parameters. Basically, any failed check in 'mtdswap_add_mtd()'.

IOW, you are solving just one of many similar issues.

I think you should rather re-work the framework a bit and make the
"->add_mtd()" call-back return an integer error code. Then we could just
return errors on error and prevent the module from being loaded.
Yang Ruirui - March 14, 2011, 8:57 a.m.
On 03/14/2011 04:43 PM, Artem Bityutskiy wrote:
> On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote:
>> From: Yang Ruirui<ruirui.r.yang@tieto.com>
>>
>> partitions can not be set after module loaded, the moduel param mode is 0444.
>>
>> this patch force module loaded with param partitions set, if user does not
>> set partitions then give out a warning and return -EINVAL
>>
>> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
>> Tested-by: Shao Yanqing<yanqing.shao@tieto.com>
>> Tested-by: Xiao Yang<yang.xiao@tieto.com>
>> ---
>>   drivers/mtd/mtdswap.c |    6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c	2011-03-14 09:36:09.283329099 +0800
>> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c	2011-03-14 09:46:30.229993534 +0800
>> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o
>>
>>   static int __init mtdswap_modinit(void)
>>   {
>> +	if (!partitions[0]) {
>> +		printk(KERN_WARNING
>> +			"Please load mtdswap with correct partitions param\n");
>> +		return -EINVAL;
>> +	}
>
> I think a similar check is done in mtdswap_add_mtd() ?
>
>

Yes, that one should be removed if this is ok. This module just waste memory without partitions set. And there's no chance to pass in the params.
Yang Ruirui - March 14, 2011, 9:12 a.m.
On 03/14/2011 04:53 PM, Artem Bityutskiy wrote:
> On Mon, 2011-03-14 at 16:57 +0800, Yang Rui Rui wrote:
>> On 03/14/2011 04:43 PM, Artem Bityutskiy wrote:
>>> On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote:
>>>> From: Yang Ruirui<ruirui.r.yang@tieto.com>
>>>>
>>>> partitions can not be set after module loaded, the moduel param
>> mode is 0444.
>>>>
>>>> this patch force module loaded with param partitions set, if user
>> does not
>>>> set partitions then give out a warning and return -EINVAL
>>>>
>>>> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
>>>> Tested-by: Shao Yanqing<yanqing.shao@tieto.com>
>>>> Tested-by: Xiao Yang<yang.xiao@tieto.com>
>>>> ---
>>>>    drivers/mtd/mtdswap.c |    6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c	2011-03-14
>> 09:36:09.283329099 +0800
>>>> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c	2011-03-14
>> 09:46:30.229993534 +0800
>>>> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o
>>>>
>>>>    static int __init mtdswap_modinit(void)
>>>>    {
>>>> +	if (!partitions[0]) {
>>>> +		printk(KERN_WARNING
>>>> +			"Please load mtdswap with correct partitions param\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> I think a similar check is done in mtdswap_add_mtd() ?
>>>
>>>
>>
>> Yes, that one should be removed if this is ok. This module just waste
>> memory without partitions set. And there's no chance to pass in the
>> params.
>
> I agree that the problem you are solving exist. But I think I do not
> agree with the solution. There are many other ways to end up with
> mtdswap module loaded but not functioning, e.g., users passes incorrect
> module parameters. Basically, any failed check in 'mtdswap_add_mtd()'.
>
> IOW, you are solving just one of many similar issues.

Right, agree.

>
> I think you should rather re-work the framework a bit and make the
> "->add_mtd()" call-back return an integer error code. Then we could just
> return errors on error and prevent the module from being loaded.
>

I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute?

Anyway, I will check the code again to see if can change as your suggestion.
Artem Bityutskiy - March 14, 2011, 9:33 a.m.
On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote:
> > I think you should rather re-work the framework a bit and make the
> > "->add_mtd()" call-back return an integer error code. Then we could just
> > return errors on error and prevent the module from being loaded.
> >
> 
> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute?

It may be possible, but would probably require some work.
Yang Ruirui - March 17, 2011, 8:50 a.m.
On 03/14/2011 05:33 PM, Artem Bityutskiy wrote:
> On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote:
>>> I think you should rather re-work the framework a bit and make the
>>> "->add_mtd()" call-back return an integer error code. Then we could just
>>> return errors on error and prevent the module from being loaded.
>>>
>>
>> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute?
>
> It may be possible, but would probably require some work.
>

mainline mtd & mtd_blkdev changes a lot since 2.6.32, but there's no
way for me to test with linux-mtd tree due to lack real hardware.

work on this with 2.6.32 seems not so necessary,
So I think I will give up, sorry about that.
Artem Bityutskiy - March 17, 2011, 8:51 a.m.
On Thu, 2011-03-17 at 16:50 +0800, Yang Rui Rui wrote:
> On 03/14/2011 05:33 PM, Artem Bityutskiy wrote:
> > On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote:
> >>> I think you should rather re-work the framework a bit and make the
> >>> "->add_mtd()" call-back return an integer error code. Then we could just
> >>> return errors on error and prevent the module from being loaded.
> >>>
> >>
> >> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute?
> >
> > It may be possible, but would probably require some work.
> >
> 
> mainline mtd & mtd_blkdev changes a lot since 2.6.32, but there's no
> way for me to test with linux-mtd tree due to lack real hardware.
> 
> work on this with 2.6.32 seems not so necessary,
> So I think I will give up, sorry about that.

No problem, but as a side note, you can always test on a PC with
nandsim. Here is some UBI-related doc, but it gives the idea:

http://www.linux-mtd.infradead.org/faq/ubi.html#L_how_debug
Yang Ruirui - March 17, 2011, 9:24 a.m.
On 03/17/2011 04:51 PM, Artem Bityutskiy wrote:
> On Thu, 2011-03-17 at 16:50 +0800, Yang Rui Rui wrote:
>> On 03/14/2011 05:33 PM, Artem Bityutskiy wrote:
>>> On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote:
>>>>> I think you should rather re-work the framework a bit and make the
>>>>> "->add_mtd()" call-back return an integer error code. Then we could just
>>>>> return errors on error and prevent the module from being loaded.
>>>>>
>>>>
>>>> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute?
>>>
>>> It may be possible, but would probably require some work.
>>>
>>
>> mainline mtd&  mtd_blkdev changes a lot since 2.6.32, but there's no
>> way for me to test with linux-mtd tree due to lack real hardware.
>>
>> work on this with 2.6.32 seems not so necessary,
>> So I think I will give up, sorry about that.
>
> No problem, but as a side note, you can always test on a PC with
> nandsim. Here is some UBI-related doc, but it gives the idea:
>
> http://www.linux-mtd.infradead.org/faq/ubi.html#L_how_debug
>

very useful for playing with mtd, thank you.

Patch

--- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c	2011-03-14 09:36:09.283329099 +0800
+++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c	2011-03-14 09:46:30.229993534 +0800
@@ -1569,6 +1569,12 @@  static struct mtd_blktrans_ops mtdswap_o
 
 static int __init mtdswap_modinit(void)
 {
+	if (!partitions[0]) {
+		printk(KERN_WARNING
+			"Please load mtdswap with correct partitions param\n");
+		return -EINVAL;
+	}
+
 	return register_mtd_blktrans(&mtdswap_ops);
 }