Re: [PATCH v2 2/3] ubifs: add ubifs_fixup_free_space()

[Artem Bityutskiy <dedekind1@gmail.com>]

On Wed, 2011-05-04 at 18:12 -0400, Matthew L. Creech wrote:
> +/**
> + * ubifs_fixup_free_space - find & fix all LEBs with free space.
> + * @c: UBIFS file-system description object
> + *
> + * This function fixes up LEBs containing free space on first mount, if the
> + * appropriate flag was set when the FS was created.  Each LEB with one or more
> + * empty pages (i.e. free-space-count > 0) is re-written, to make sure the
> + * free space is actually erased.  This is necessary for some NAND chips, since
> + * the free space may have been programmed like real "0xff" data (generating a
> + * non-0xff ECC), causing future writes to the not-really-erased pages to
> + * behave badly.  After fixup, the superblock flag is removed so that this is
> + * skipped for all future mounts.
> + */
> +int ubifs_fixup_free_space(struct ubifs_info *c)
> +{
> +	int err = 0, sup_flags = 0;
> +	struct ubifs_sb_node *sup;
> +
> +	ubifs_assert(c->space_fixup);
> +	ubifs_assert(!c->ro_mount);
> +
> +	ubifs_msg("free-space fixup needed");
> +
> +	err = fixup_free_space(c);
> +	if (err)
> +		goto out;
> +
> +	sup = ubifs_read_sb_node(c);
> +	if (IS_ERR(sup)) {
> +		err = PTR_ERR(sup);
> +		goto out;
> +	}

This function will allocate 'sup' and you never free it. Please, do
kfree(sup) at "out:".

I've noticed the same bug in 'ubifs_remount_rw()' and just fixed it -
see patch [7/7] in the series I've sent few minutes ago to the mailing
list.

> +
> +	/* Free-space fixup is no longer required */
> +	c->space_fixup = 0;
> +
> +	/* Set new flags, omitting free-space fixup */
> +	sup_flags = 0;
> +	if (c->big_lpt)
> +		sup_flags |= UBIFS_FLG_BIGLPT;
> +	sup->flags = cpu_to_le32(sup_flags);

Could you please change this piece of code to something like

sup->flags &= cpu_to_le32(~UBIFS_FLG_SPACE_FIXUP);

I mean, obviously your piece of code is error-prone because if someone
adds yet another SB flag some day, he might miss your code which clears
it. IOW, you should touch only the UBIFS_FLG_SPACE_FIXUP bit and nothing
else.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)