Re: [PATCH] jffs2:freely allocate memory when parameters are invalid

[Al Viro <viro@xxxxxxxxxxxxxxxxxx>]

On Fri, Sep 20, 2019 at 10:13:53PM +0800, Xiaoming Ni wrote:
> 1. drivers/mtd/mtdsuper.c
> mount_mtd_aux() {
> ....
>    /* jffs2_sb_info is allocated in jffs2_fill_super, */
>     ret = fill_super(sb, data, flags & SB_SILENT ? 1 : 0);
>     if (ret < 0) {
>         deactivate_locked_super(sb); /* If the parameter is wrong, release it here*/
>         return ERR_PTR(ret);
>     }
> ...
> }
> 
> 2. fs/super.c
> deactivate_locked_super()
> ---> fs->kill_sb(s);
> 
> 3. fs/jffs2/super.c
>  jffs2_kill_sb()
>     kfree(c); /*release jffs2_sb_info allocated by jffs2_fill_super here
> 
> Here memory allocation and release,
> experienced the function of mount_mtd_aux/deactivate_locked_super/jffs2_kill_sb three different files,
> the path is relatively long,
> if any of the three functions between the errors,

If any of the three functions _what_?

> it will cause problems (such as memory leaks)
 
> I still think this is easier to understand:
>  Free the memory allocated by the current function in the failed branch

No.  Again, "failed" branch is going to be practically untested during
any later code changes.  The more you have to do in those, the faster they
rots.  And they _do_ rot - we'd seen that happening again and again.

As a general rule, the fewer cleanups are required on failure exits, the
better off we are.

> static void jffs2_kill_sb(struct super_block *sb)
> {
>     struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
>     if (c && !sb_rdonly(sb))
> 		/* If sb is not read only,
> 		 * then jffs2_stop_garbage_collect_thread() will be executed
> 		 * when the jffs2_fill_super parameter is invalid.
> 		 */
>         jffs2_stop_garbage_collect_thread(c);
>     kill_mtd_super(sb);
>     kfree(c);
> }
> 
> void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c)
> {
>     int wait = 0;
> 	/* When the jffs2_fill_super parameter is invalid,
> 	 * this lock is not initialized.
> 	 * Is this a code problem ?
> 	 */
>     spin_lock(&c->erase_completion_lock);

Not in practice and gone in mainline this cycle.  But yes, those initializations
should've been done prior to any failure exits -
"jffs2: free jffs2_sb_info through jffs2_kill_sb()" ought to have moved them
prior to the call of jffs2_parse_options().

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/