linux-kernel - Re: [PATCH 4/4] UBI: replace MTD_UBI_BEB

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]

Message-ID: <CACQ1gAhFJ6AKYJpc1ZiHm+3Y=APnwUmVL3f0KW=OFMDMWzCVJg@mail.gmail.com>
Date:	Thu, 16 Aug 2012 12:07:01 +0200
From:	Richard Genoud <richard.genoud@...il.com>
To:	Shmulik Ladkani <shmulik.ladkani@...il.com>
Cc:	Artem Bityutskiy <dedekind1@...il.com>,
	linux-mtd@...ts.infradead.org,
	David Woodhouse <dwmw2@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] UBI: replace MTD_UBI_BEB_LIMIT with user-space parameter

2012/8/16 Shmulik Ladkani <shmulik.ladkani@...il.com>:
> Hi Richard,
>
> Sorry for reviewing this late...
>
> On Tue, 10 Jul 2012 18:23:42 +0200 Richard Genoud <richard.genoud@...il.com> wrote:
>> -config MTD_UBI_BEB_LIMIT
>> -     int "Maximum expected bad eraseblocks per 1024 eraseblocks"
>> -     default 20
>> -     range 2 256
>
> I see some benefit keeping the config.
>
> For the simplest systems (those having one ubi device) that need a limit
> *other* than the default (20 per 1024), they can simply set the config
> to their chosen value, as they were used to.
>
> With you approach, these system MUST pass the limit parameter via the
> ioctl / module-parameter.
That's right.
We can add a kernel config option to change the max_beb_per1024
default value (actually, this is almost the patch I send first).
But I see something disturbing with that:
It means that an ubi_attach call from userspace, without specifying
max_beb_per1024, won't have the same result, depending of the default
config value the kernel has been compiled with.
(Or maybe this behavior is acceptable).

>> +static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
>> +{
>> +     int device_peb_count;
>> +     uint64_t device_size;
>> +     int beb_limit = 0;
>> +
>> +     /* this has already been checked in ioctl */
>> +     if (max_beb_per1024 <= 0)
>> +             goto out;
>
> Can you explain how 'max_beb_per1024 <= 0' may happen?
>
> It seems that all of your calls to 'ubi_attach_mtd_dev' pass a positive
> max_beb_per1024 value (the default is always set). See your
> 'ubi_mtd_param_parse' and 'ctrl_cdev_ioctl'. Am I missing something?
Actually it can. But it's because I made a mistake:
p->max_beb_per1024 = bytes_str_to_int(tokens[2]);
=> I didn't check the return value of this. It can fail, and if it
does the return value is >0.
I'm going to change that.

>
> Also, since max_beb_per1024 is always set, how one may specify a zero
> limit?
You can't.
Do you think we need that ?
0 should be kept for "default value", not to break user-space.
But we can use another value for 0, like 1024 or 2048.
(but honestly, I think this will add complexity for an unlikely configuration.)

>
> Regards,
> Shmulik

Regards,
Richard.


-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/