linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] JFS: fix memleak in jfs_mount
@ 2021-09-04  2:37 Dongliang Mu
  2021-09-23  0:53 ` Dongliang Mu
  0 siblings, 1 reply; 5+ messages in thread
From: Dongliang Mu @ 2021-09-04  2:37 UTC (permalink / raw)
  To: Dave Kleikamp; +Cc: Dongliang Mu, jfs-discussion, linux-kernel

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>
---
v1->v2: change "return rc" to "goto out"; fix one coding style
 fs/jfs/jfs_mount.c | 51 ++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
index 5d7d7170c03c..aa4ff7bcaff2 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;
+		goto out;
 	}
 
 	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,34 @@ 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) {
+	if (ipaimap2)
 		diUnmount(ipaimap2, 1);
+err_ipaimap2:
+	/* close aggregate inodes */
+	if (ipaimap2)
 		diFreeSpecial(ipaimap2);
-	}
-
-      errout35:
-
-	/* close aggregate block allocation map */
+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);
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] JFS: fix memleak in jfs_mount
  2021-09-04  2:37 [PATCH v2] JFS: fix memleak in jfs_mount Dongliang Mu
@ 2021-09-23  0:53 ` Dongliang Mu
  2021-09-23 14:54   ` Dave Kleikamp
  0 siblings, 1 reply; 5+ messages in thread
From: Dongliang Mu @ 2021-09-23  0:53 UTC (permalink / raw)
  To: Dave Kleikamp; +Cc: jfs-discussion, linux-kernel

Any update on this thread?

On Sat, Sep 4, 2021 at 10:38 AM Dongliang Mu <mudongliangabcd@gmail.com> 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>
> ---
> v1->v2: change "return rc" to "goto out"; fix one coding style
>  fs/jfs/jfs_mount.c | 51 ++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
> index 5d7d7170c03c..aa4ff7bcaff2 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;
> +               goto out;
>         }
>
>         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,34 @@ 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) {
> +       if (ipaimap2)
>                 diUnmount(ipaimap2, 1);
> +err_ipaimap2:
> +       /* close aggregate inodes */
> +       if (ipaimap2)
>                 diFreeSpecial(ipaimap2);
> -       }
> -
> -      errout35:
> -
> -       /* close aggregate block allocation map */
> +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);
>
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] JFS: fix memleak in jfs_mount
  2021-09-23  0:53 ` Dongliang Mu
@ 2021-09-23 14:54   ` Dave Kleikamp
  2021-11-08  1:10     ` Dongliang Mu
  0 siblings, 1 reply; 5+ messages in thread
From: Dave Kleikamp @ 2021-09-23 14:54 UTC (permalink / raw)
  To: Dongliang Mu; +Cc: jfs-discussion, linux-kernel

On 9/22/21 19:53, Dongliang Mu wrote:
> Any update on this thread?

Sorry for taking this long to get back to it. The patch looks good. It 
should show up in linux-next shortly.

Shaggy

> 
> On Sat, Sep 4, 2021 at 10:38 AM Dongliang Mu <mudongliangabcd@gmail.com> 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>
>> ---
>> v1->v2: change "return rc" to "goto out"; fix one coding style
>>   fs/jfs/jfs_mount.c | 51 ++++++++++++++++++++--------------------------
>>   1 file changed, 22 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
>> index 5d7d7170c03c..aa4ff7bcaff2 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;
>> +               goto out;
>>          }
>>
>>          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,34 @@ 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) {
>> +       if (ipaimap2)
>>                  diUnmount(ipaimap2, 1);
>> +err_ipaimap2:
>> +       /* close aggregate inodes */
>> +       if (ipaimap2)
>>                  diFreeSpecial(ipaimap2);
>> -       }
>> -
>> -      errout35:
>> -
>> -       /* close aggregate block allocation map */
>> +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);
>>
>> --
>> 2.25.1
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] JFS: fix memleak in jfs_mount
  2021-09-23 14:54   ` Dave Kleikamp
@ 2021-11-08  1:10     ` Dongliang Mu
  2021-11-08  1:15       ` Dongliang Mu
  0 siblings, 1 reply; 5+ messages in thread
From: Dongliang Mu @ 2021-11-08  1:10 UTC (permalink / raw)
  To: Dave Kleikamp; +Cc: jfs-discussion, linux-kernel

On Thu, Sep 23, 2021 at 10:54 PM Dave Kleikamp <dave.kleikamp@oracle.com> wrote:
>
> On 9/22/21 19:53, Dongliang Mu wrote:
> > Any update on this thread?
>
> Sorry for taking this long to get back to it. The patch looks good. It
> should show up in linux-next shortly.
>

Hi Dave,

In my local syzkaller testing, the upstream is still vulnerable to
this patch. How does the patch applied in linux-next propagate to
upstream?

> Shaggy
>
> >
> > On Sat, Sep 4, 2021 at 10:38 AM Dongliang Mu <mudongliangabcd@gmail.com> 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>
> >> ---
> >> v1->v2: change "return rc" to "goto out"; fix one coding style
> >>   fs/jfs/jfs_mount.c | 51 ++++++++++++++++++++--------------------------
> >>   1 file changed, 22 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
> >> index 5d7d7170c03c..aa4ff7bcaff2 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;
> >> +               goto out;
> >>          }
> >>
> >>          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,34 @@ 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) {
> >> +       if (ipaimap2)
> >>                  diUnmount(ipaimap2, 1);
> >> +err_ipaimap2:
> >> +       /* close aggregate inodes */
> >> +       if (ipaimap2)
> >>                  diFreeSpecial(ipaimap2);
> >> -       }
> >> -
> >> -      errout35:
> >> -
> >> -       /* close aggregate block allocation map */
> >> +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);
> >>
> >> --
> >> 2.25.1
> >>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] JFS: fix memleak in jfs_mount
  2021-11-08  1:10     ` Dongliang Mu
@ 2021-11-08  1:15       ` Dongliang Mu
  0 siblings, 0 replies; 5+ messages in thread
From: Dongliang Mu @ 2021-11-08  1:15 UTC (permalink / raw)
  To: Dave Kleikamp; +Cc: jfs-discussion, linux-kernel

On Mon, Nov 8, 2021 at 9:10 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Thu, Sep 23, 2021 at 10:54 PM Dave Kleikamp <dave.kleikamp@oracle.com> wrote:
> >
> > On 9/22/21 19:53, Dongliang Mu wrote:
> > > Any update on this thread?
> >
> > Sorry for taking this long to get back to it. The patch looks good. It
> > should show up in linux-next shortly.
> >
>
> Hi Dave,
>
> In my local syzkaller testing, the upstream is still vulnerable to
> this patch. How does the patch applied in linux-next propagate to
> upstream?

Please ignore it. I have found the commit in the upstream.

>
> > Shaggy
> >
> > >
> > > On Sat, Sep 4, 2021 at 10:38 AM Dongliang Mu <mudongliangabcd@gmail.com> 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>
> > >> ---
> > >> v1->v2: change "return rc" to "goto out"; fix one coding style
> > >>   fs/jfs/jfs_mount.c | 51 ++++++++++++++++++++--------------------------
> > >>   1 file changed, 22 insertions(+), 29 deletions(-)
> > >>
> > >> diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
> > >> index 5d7d7170c03c..aa4ff7bcaff2 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;
> > >> +               goto out;
> > >>          }
> > >>
> > >>          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,34 @@ 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) {
> > >> +       if (ipaimap2)
> > >>                  diUnmount(ipaimap2, 1);
> > >> +err_ipaimap2:
> > >> +       /* close aggregate inodes */
> > >> +       if (ipaimap2)
> > >>                  diFreeSpecial(ipaimap2);
> > >> -       }
> > >> -
> > >> -      errout35:
> > >> -
> > >> -       /* close aggregate block allocation map */
> > >> +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);
> > >>
> > >> --
> > >> 2.25.1
> > >>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-11-08  1:15 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-04  2:37 [PATCH v2] JFS: fix memleak in jfs_mount Dongliang Mu
2021-09-23  0:53 ` Dongliang Mu
2021-09-23 14:54   ` Dave Kleikamp
2021-11-08  1:10     ` Dongliang Mu
2021-11-08  1:15       ` Dongliang Mu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).