| 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: <20180420095035.i6euhg66kmmvqx4w@qschulz>
Date: Fri, 20 Apr 2018 11:50:36 +0200
From: Quentin Schulz <quentin.schulz@...tlin.com>
To: Richard Weinberger <richard@....at>
Cc: dedekind1@...il.com, dwmw2@...radead.org,
computersforpeace@...il.com, boris.brezillon@...tlin.com,
marek.vasut@...il.com, linux-mtd@...ts.infradead.org,
linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com
Subject: Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC
check when attaching ubi vol
Hi Richard,
On Fri, Apr 20, 2018 at 11:37:07AM +0200, Richard Weinberger wrote:
> Quentin,
>
> Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz:
> > There's already ECC on NAND pages so there may be no need for one to
> > check the CRC of a UBI volume.
> >
> > Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> > when attaching a UBI volume.
> >
> > This also drastically speeds kernel boot by removing a potentially
> > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> > for a ~20MB UBI volume used as rootfs.
>
> You mean at *open* time of a *static* UBI volume?
> So I guess the use case is having a read-only filesystem on top of ubiblock?
>
Turns out I forgot some important information and poorly worded my
commit log :)
Yes sorry, I have a squashfs (read-only by design) as the image of one
static UBI volume.
And yes, I meant when opening a static UBI volume.
Quentin
> > Signed-off-by: Quentin Schulz <quentin.schulz@...tlin.com>
> > ---
> > drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
> > drivers/mtd/ubi/ubi.h | 6 ++++-
> > drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++-
> > 3 files changed, 99 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
> > index d4b2e87..d604cd5 100644
> > --- a/drivers/mtd/ubi/kapi.c
> > +++ b/drivers/mtd/ubi/kapi.c
> > @@ -28,6 +28,9 @@
> > #include <asm/div64.h>
> > #include "ubi.h"
> >
> > +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> > +int ubinocheck_devs;
> > +
> > /**
> > * ubi_do_get_device_info - get information about UBI device.
> > * @ubi: UBI device description object
> > @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb)
> > return blocking_notifier_chain_unregister(&ubi_notifiers, nb);
> > }
> > EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier);
> > +
> > +static int __init ubinocheckvol_set_param(const char *val,
> > + const struct kernel_param *kp)
> > +{
> > + int i, ret;
> > + size_t len;
> > + struct ubivol_param *param;
> > + char buf[UBIVOL_PARAM_LEN];
> > + char *pbuf = &buf[0];
> > + char *tokens[UBIVOL_PARAM_COUNT];
> > +
> > + if (!val)
> > + return -EINVAL;
> > +
> > + len = strnlen(val, UBIVOL_PARAM_LEN);
> > + if (len == 0) {
> > + pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n");
> > + return 0;
> > + }
> > +
> > + if (len == UBIVOL_PARAM_LEN) {
> > + pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n",
> > + val, UBIVOL_PARAM_LEN);
> > + return -EINVAL;
> > + }
> > +
> > + strcpy(buf, val);
> > +
> > + /* Get rid of the final newline */
> > + if (buf[len - 1] == '\n')
> > + buf[len - 1] = '\0';
> > +
> > + for (i = 0; i < UBIVOL_PARAM_COUNT; i++)
> > + tokens[i] = strsep(&pbuf, ",");
> > +
> > + param = &ubinocheck_param[ubinocheck_devs];
> > + if (tokens[1]) {
> > + /* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */
> > + ret = kstrtoint(tokens[0], 10, ¶m->ubi_num);
> > + if (ret < 0)
> > + return -EINVAL;
> > +
> > + /* Second param can be a number or a name */
> > + ret = kstrtoint(tokens[1], 10, ¶m->vol_id);
> > + if (ret < 0) {
> > + param->vol_id = -1;
> > + strcpy(param->name, tokens[1]);
> > + }
> > + }
> >
>
> Do we really need this per volume? If your flash is trustworthy, it should not matter.
> Having it per UBI instance instead of volume would make the code less complicated.
>
> > + ubinocheck_devs++;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct kernel_param_ops ubinocheckvol_param_ops = {
> > + .set = ubinocheckvol_set_param,
> > +};
> > +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0);
> > +MODULE_PARM_DESC(nocheck,
> > + "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n"
> > + "Multiple \"nocheck\" parameters may be specified.\n"
> > + "Examples\n"
> > + "Using the UBI device, and the volume name:\n"
> > + "ubi.nocheck=0,rootfs\n"
> > + "Using both UBI device number and UBI volume number:\n"
> > + "ubi.nocheck=0,0\n");
> > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> > index 4cc6ec9..2bd89b4 100644
> > --- a/drivers/mtd/ubi/ubi.h
> > +++ b/drivers/mtd/ubi/ubi.h
> > @@ -825,6 +825,12 @@ struct ubivol_param {
> > char name[UBIVOL_PARAM_LEN + 1];
> > };
> >
> > +/* Numbers of elements set in the @ubinocheck_param array */
> > +extern int ubinocheck_devs;
> > +
> > +/* MTD devices specification parameters */
> > +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> > +
> > #include "debug.h"
> >
> > extern struct kmem_cache *ubi_wl_entry_slab;
> > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> > index 263743e..06b6cfd 100644
> > --- a/drivers/mtd/ubi/vtbl.c
> > +++ b/drivers/mtd/ubi/vtbl.c
> > @@ -534,9 +534,10 @@ static int init_volumes(struct ubi_device *ubi,
> > const struct ubi_attach_info *ai,
> > const struct ubi_vtbl_record *vtbl)
> > {
> > - int i, reserved_pebs = 0;
> > + int i, j, reserved_pebs = 0;
> > struct ubi_ainf_volume *av;
> > struct ubi_volume *vol;
> > + struct ubivol_param *param;
> >
> > for (i = 0; i < ubi->vtbl_slots; i++) {
> > cond_resched();
> > @@ -620,6 +621,27 @@ static int init_volumes(struct ubi_device *ubi,
> > (long long)(vol->used_ebs - 1) * vol->usable_leb_size;
> > vol->used_bytes += av->last_data_size;
> > vol->last_eb_bytes = av->last_data_size;
> > +
> > + for (j = 0; j < ubinocheck_devs; j++) {
> > + param = &ubinocheck_param[j];
> > +
> > + if (vol->ubi->ubi_num != param->ubi_num)
> > + continue;
> > + if (vol->vol_id == param->vol_id) {
> > + ubi_msg(vol->ubi,
> > + "skipping CRC check for volume %d",
> > + vol->vol_id);
> > + vol->checked = true;
>
> Please don't abuse the checked flag.
> A new one a la "skip_check" does not hurt.
> But again, I don't think we need it per volume.
>
> Also don't forget to add the nocheck parameter to the ioctl() interface, such that
> you can specify it also with ubiattach...
>
> Thanks,
> //richard
Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)
Powered by blists - more mailing lists