Re: UBIFS: make space fixup work in the remount case

Re: UBIFS: make space fixup work in the remount case

[Artem Bityutskiy]

On Mon, 2013-04-15 at 12:59 +0300, Dan Carpenter wrote:
>   1571          if (c->space_fixup) {
>   1572                  err = ubifs_fixup_free_space(c);
>   1573                  if (err)
>   1574                          return err;
>                                 ^^^^^^^^^^
> Shouldn't this be a "goto out?".  We're holding the c->umount_mutex.

Hi Dan, I fixed this up and answered the reporter:
https://patchwork.kernel.org/patch/2284681/

It is now 'goto out'. Thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: UBIFS: make space fixup work in the remount case

[Dan Carpenter]

On Mon, Apr 15, 2013 at 01:11:33PM +0300, Artem Bityutskiy wrote:
> On Mon, 2013-04-15 at 12:59 +0300, Dan Carpenter wrote:
> >   1571          if (c->space_fixup) {
> >   1572                  err = ubifs_fixup_free_space(c);
> >   1573                  if (err)
> >   1574                          return err;
> >                                 ^^^^^^^^^^
> > Shouldn't this be a "goto out?".  We're holding the c->umount_mutex.
> 
> Hi Dan, I fixed this up and answered the reporter:
> https://patchwork.kernel.org/patch/2284681/
> 
> It is now 'goto out'. Thanks!

Huh...  That wasn't who I expected to report it.  :P  It's good to
know that these kind of bugs don't last very long.

regards,
dan carpenter

Re: UBIFS: make space fixup work in the remount case

[Artem Bityutskiy]

On Mon, 2013-04-15 at 13:19 +0300, Dan Carpenter wrote:
> On Mon, Apr 15, 2013 at 01:11:33PM +0300, Artem Bityutskiy wrote:
> > On Mon, 2013-04-15 at 12:59 +0300, Dan Carpenter wrote:
> > >   1571          if (c->space_fixup) {
> > >   1572                  err = ubifs_fixup_free_space(c);
> > >   1573                  if (err)
> > >   1574                          return err;
> > >                                 ^^^^^^^^^^
> > > Shouldn't this be a "goto out?".  We're holding the c->umount_mutex.
> > 
> > Hi Dan, I fixed this up and answered the reporter:
> > https://patchwork.kernel.org/patch/2284681/
> > 
> > It is now 'goto out'. Thanks!
> 
> Huh...  That wasn't who I expected to report it.  :P  It's good to
> know that these kind of bugs don't last very long.

The story actually was that I noticed this myself after running Aiaiai
and fixed it up "in-place", and then several people reported, and I
answered to one of them.

-- 
Best Regards,
Artem Bityutskiy

Re: UBIFS: make space fixup work in the remount case

[Dan Carpenter]

On Mon, Apr 15, 2013 at 01:39:06PM +0300, Artem Bityutskiy wrote:
> On Mon, 2013-04-15 at 13:19 +0300, Dan Carpenter wrote:
> > On Mon, Apr 15, 2013 at 01:11:33PM +0300, Artem Bityutskiy wrote:
> > > On Mon, 2013-04-15 at 12:59 +0300, Dan Carpenter wrote:
> > > >   1571          if (c->space_fixup) {
> > > >   1572                  err = ubifs_fixup_free_space(c);
> > > >   1573                  if (err)
> > > >   1574                          return err;
> > > >                                 ^^^^^^^^^^
> > > > Shouldn't this be a "goto out?".  We're holding the c->umount_mutex.
> > > 
> > > Hi Dan, I fixed this up and answered the reporter:
> > > https://patchwork.kernel.org/patch/2284681/
> > > 
> > > It is now 'goto out'. Thanks!
> > 
> > Huh...  That wasn't who I expected to report it.  :P  It's good to
> > know that these kind of bugs don't last very long.
> 
> The story actually was that I noticed this myself after running Aiaiai
> and fixed it up "in-place", and then several people reported, and I
> answered to one of them.

Hi Artem,

This showed up in my smatch output again today.  Whatever happened with
this.  I reported it last Jun and last Nov as well apparently.

regards,
dan carpenter

Re: UBIFS: make space fixup work in the remount case

[Artem Bityutskiy]

On Tue, 2014-04-01 at 16:39 +0300, Dan Carpenter wrote:
> On Mon, Apr 15, 2013 at 01:39:06PM +0300, Artem Bityutskiy wrote:
> > On Mon, 2013-04-15 at 13:19 +0300, Dan Carpenter wrote:
> > > On Mon, Apr 15, 2013 at 01:11:33PM +0300, Artem Bityutskiy wrote:
> > > > On Mon, 2013-04-15 at 12:59 +0300, Dan Carpenter wrote:
> > > > >   1571          if (c->space_fixup) {
> > > > >   1572                  err = ubifs_fixup_free_space(c);
> > > > >   1573                  if (err)
> > > > >   1574                          return err;
> > > > >                                 ^^^^^^^^^^
> > > > > Shouldn't this be a "goto out?".  We're holding the c->umount_mutex.
> > > > 
> > > > Hi Dan, I fixed this up and answered the reporter:
> > > > https://patchwork.kernel.org/patch/2284681/
> > > > 
> > > > It is now 'goto out'. Thanks!
> > > 
> > > Huh...  That wasn't who I expected to report it.  :P  It's good to
> > > know that these kind of bugs don't last very long.
> > 
> > The story actually was that I noticed this myself after running Aiaiai
> > and fixed it up "in-place", and then several people reported, and I
> > answered to one of them.
> 
> Hi Artem,
> 
> This showed up in my smatch output again today.  Whatever happened with
> this.  I reported it last Jun and last Nov as well apparently.

Hmm, I was sure I fixed this, but it is still there.

OK, created and pushed a fix to the master branch. It won't hit
linux-next so far, I'll propagate it there once 3.15-rc1 or -rc2 is out.
Thanks!


From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Mon, 7 Apr 2014 13:39:06 +0300
Subject: [PATCH] UBIFS: fix remount error path

Dan's "smatch" checker found out that there was a bug in the error path of the
'ubifs_remount_rw()' function. Instead of jumping to the "out" label which
cleans-things up, we just returned.

This patch fixes the problem.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/ubifs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 5ded849..7e71ac9 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1556,7 +1556,7 @@ static int ubifs_remount_rw(struct ubifs_info *c)
        if (c->space_fixup) {
                err = ubifs_fixup_free_space(c);
                if (err)
-                       return err;
+                       goto out;
        }
 
        err = check_free_space(c);
-- 
1.8.5.3

-- 
Best Regards,
Artem Bityutskiy