Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210818102612.864127-1-mudongliangabcd@gmail.com> <fc1dae76-eac1-e6f4-2ba6-f49e15ad0b46@oracle.com>
In-Reply-To: <fc1dae76-eac1-e6f4-2ba6-f49e15ad0b46@oracle.com>
From:   Dongliang Mu <mudongliangabcd@gmail.com>
Date:   Sat, 4 Sep 2021 10:26:25 +0800
Message-ID: <CAD-N9QWihGFBf8WgwynNe295TC5rdyaO3kXgW+-MvqF4O5xvKg@mail.gmail.com>
Subject: Re: [PATCH] JFS: fix memleak in jfs_mount
To:     Dave Kleikamp <dave.kleikamp@oracle.com>
Cc:     jfs-discussion@lists.sourceforge.net,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Sep 4, 2021 at 5:51 AM Dave Kleikamp <dave.kleikamp@oracle.com> wrote:
>
> Thanks for waiting. I have just a couple comments, but this looks good.
> I appreciate the clean up.
>
> On 8/18/21 5:25 AM, Dongliang Mu wrote:
> > In jfs_mount, when diMount(ipaimap2) fails, it goes to errout35. However,
> > the following code does not free ipaimap2 allocated by diReadSpecial.
> >
> > Fix this by refactoring the error handling code of jfs_mount. To be
> > specific, modify the lable name and free ipaimap2 when the above error
> > ocurrs.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >   fs/jfs/jfs_mount.c | 53 +++++++++++++++++++---------------------------
> >   1 file changed, 22 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
> > index 5d7d7170c03c..638a4ecc4069 100644
> > --- a/fs/jfs/jfs_mount.c
> > +++ b/fs/jfs/jfs_mount.c
> > @@ -81,14 +81,14 @@ int jfs_mount(struct super_block *sb)
> >        * (initialize mount inode from the superblock)
> >        */
> >       if ((rc = chkSuper(sb))) {
> > -             goto errout20;
> > +             return rc;
>
> This may have been intentional, but it isn't mentioned in the patch header.
>
> If chkSuper fails, we will no longer call
>         jfs_err("Mount JFS Failure: %d", rc);
>
> I don't necessarily see this as a bad thing. In many cases, chkSuper
> prints a more helpful message. In the case where it silently fails, it's
> not even recognizing the superblock as a supported version of JFS and
> this message isn't particularly helpful. In fact this jfs_err()
> statement might be best deleted in its entirety.

I see. Let me change "return rc" to "goto out", the same as "goto errout20".

This will put back the jfs_err message output.

>
> >       }
> >
> >       ipaimap = diReadSpecial(sb, AGGREGATE_I, 0);
> >       if (ipaimap == NULL) {
> >               jfs_err("jfs_mount: Failed to read AGGREGATE_I");
> >               rc = -EIO;
> > -             goto errout20;
> > +             goto out;
> >       }
> >       sbi->ipaimap = ipaimap;
> >
> > @@ -99,7 +99,7 @@ int jfs_mount(struct super_block *sb)
> >        */
> >       if ((rc = diMount(ipaimap))) {
> >               jfs_err("jfs_mount: diMount(ipaimap) failed w/rc = %d", rc);
> > -             goto errout21;
> > +             goto err_ipaimap;
> >       }
> >
> >       /*
> > @@ -108,7 +108,7 @@ int jfs_mount(struct super_block *sb)
> >       ipbmap = diReadSpecial(sb, BMAP_I, 0);
> >       if (ipbmap == NULL) {
> >               rc = -EIO;
> > -             goto errout22;
> > +             goto err_umount_ipaimap;
> >       }
> >
> >       jfs_info("jfs_mount: ipbmap:0x%p", ipbmap);
> > @@ -120,7 +120,7 @@ int jfs_mount(struct super_block *sb)
> >        */
> >       if ((rc = dbMount(ipbmap))) {
> >               jfs_err("jfs_mount: dbMount failed w/rc = %d", rc);
> > -             goto errout22;
> > +             goto err_ipbmap;
> >       }
> >
> >       /*
> > @@ -139,7 +139,7 @@ int jfs_mount(struct super_block *sb)
> >               if (!ipaimap2) {
> >                       jfs_err("jfs_mount: Failed to read AGGREGATE_I");
> >                       rc = -EIO;
> > -                     goto errout35;
> > +                     goto err_umount_ipbmap;
> >               }
> >               sbi->ipaimap2 = ipaimap2;
> >
> > @@ -151,7 +151,7 @@ int jfs_mount(struct super_block *sb)
> >               if ((rc = diMount(ipaimap2))) {
> >                       jfs_err("jfs_mount: diMount(ipaimap2) failed, rc = %d",
> >                               rc);
> > -                     goto errout35;
> > +                     goto err_ipaimap2;
> >               }
> >       } else
> >               /* Secondary aggregate inode table is not valid */
> > @@ -168,7 +168,7 @@ int jfs_mount(struct super_block *sb)
> >               jfs_err("jfs_mount: Failed to read FILESYSTEM_I");
> >               /* open fileset secondary inode allocation map */
> >               rc = -EIO;
> > -             goto errout40;
> > +             goto err_umount_ipaimap2;
> >       }
> >       jfs_info("jfs_mount: ipimap:0x%p", ipimap);
> >
> > @@ -178,41 +178,32 @@ int jfs_mount(struct super_block *sb)
> >       /* initialize fileset inode allocation map */
> >       if ((rc = diMount(ipimap))) {
> >               jfs_err("jfs_mount: diMount failed w/rc = %d", rc);
> > -             goto errout41;
> > +             goto err_ipimap;
> >       }
> >
> > -     goto out;
> > +     return rc;
> >
> >       /*
> >        *      unwind on error
> >        */
> > -      errout41:              /* close fileset inode allocation map inode */
> > +err_ipimap:
> > +     /* close fileset inode allocation map inode */
> >       diFreeSpecial(ipimap);
> > -
> > -      errout40:              /* fileset closed */
> > -
> > +err_umount_ipaimap2:
> >       /* close secondary aggregate inode allocation map */
> > -     if (ipaimap2) {
> > -             diUnmount(ipaimap2, 1);
> > -             diFreeSpecial(ipaimap2);
> > -     }
> > -
> > -      errout35:
> > -
> > -     /* close aggregate block allocation map */
> > +     if (ipaimap2) diUnmount(ipaimap2, 1);
>
> Coding style: this should be split between two lines:
>         if (ipaimap2)
>                 diUnmount(ipaimap2, 1);
>
> > +err_ipaimap2:
> > +     /* close aggregate inodes */
> > +     if (ipaimap2) diFreeSpecial(ipaimap2);
>
> Same here.

Will modify it in the v2 version.

>
> > +err_umount_ipbmap:   /* close aggregate block allocation map */
> >       dbUnmount(ipbmap, 1);
> > +err_ipbmap:          /* close aggregate inodes */
> >       diFreeSpecial(ipbmap);
> > -
> > -      errout22:              /* close aggregate inode allocation map */
> > -
> > +err_umount_ipaimap:  /* close aggregate inode allocation map */
> >       diUnmount(ipaimap, 1);
> > -
> > -      errout21:              /* close aggregate inodes */
> > +err_ipaimap:         /* close aggregate inodes */
> >       diFreeSpecial(ipaimap);
> > -      errout20:              /* aggregate closed */
> > -
> > -      out:
> > -
> > +out:
> >       if (rc)
> >               jfs_err("Mount JFS Failure: %d", rc);
> >
> >