Re: f2fs crypto: add symlink encryption

Re: f2fs crypto: add symlink encryption

[Dan Carpenter]

Hello Jaegeuk Kim,

The patch 24d584edddca: "f2fs crypto: add symlink encryption" from
Apr 29, 2015, leads to the following static checker warning:

	fs/f2fs/namei.c:487 f2fs_symlink()
	warn: calling kfree() when 'sd' is always NULL.

fs/f2fs/namei.c
   484          f2fs_fname_crypto_free_buffer(&disk_link);
   485          return err;
   486  out:
   487          kfree(sd);
                ^^^^^^^^^
Freeing a NULL pointer is harmless but I have a static checker warning
for it because it can indicate confusion or typos.

   488          f2fs_fname_crypto_free_buffer(&disk_link);
   489          handle_failed_inode(inode);
   490          return err;
   491  }

"out" labels are bad.

The name is meaningless.  If it says "goto free_sd;" that's useful and
you know what it does without scrolling down and losing your place.
But with out: labels you can't know.

Sometimes out labels do nothing.

	out:
		return err;

These are supposed to prevent return with lock held bugs etc, but if you
look through the git log it has historically not been effective.  It
also introduces "forgot to set the error code" bugs.

Sometime it does everything like in this case and that's very bug prone.
For example, shouldn't we release f2fs_lock_op(sbi) when f2fs_add_link()
fails earlier?

Sometimes it does one thing which is good, but the name is just lazy.

-out:
+unlock:
	mutext_unlock();
	return err;

Btw shouldn't we earlier if page_symlink() fails?

regards,
dan carpenter


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: f2fs crypto: add symlink encryption

[Jaegeuk Kim]

Hi Dan,

Thank you for pointing this out. :)

On Tue, May 12, 2015 at 03:09:17PM +0300, Dan Carpenter wrote:
> Hello Jaegeuk Kim,
> 
> The patch 24d584edddca: "f2fs crypto: add symlink encryption" from
> Apr 29, 2015, leads to the following static checker warning:
> 
> 	fs/f2fs/namei.c:487 f2fs_symlink()
> 	warn: calling kfree() when 'sd' is always NULL.
> 
> fs/f2fs/namei.c
>    484          f2fs_fname_crypto_free_buffer(&disk_link);
>    485          return err;
>    486  out:
>    487          kfree(sd);
>                 ^^^^^^^^^
> Freeing a NULL pointer is harmless but I have a static checker warning
> for it because it can indicate confusion or typos.

V2 resolved this.

> 
>    488          f2fs_fname_crypto_free_buffer(&disk_link);
>    489          handle_failed_inode(inode);
>    490          return err;
>    491  }
> 
> "out" labels are bad.
> 
> The name is meaningless.  If it says "goto free_sd;" that's useful and
> you know what it does without scrolling down and losing your place.
> But with out: labels you can't know.
> 
> Sometimes out labels do nothing.
> 
> 	out:
> 		return err;
> 
> These are supposed to prevent return with lock held bugs etc, but if you
> look through the git log it has historically not been effective.  It
> also introduces "forgot to set the error code" bugs.

Agreed.
Let me think about changing labels across the whole areas.

> 
> Sometime it does everything like in this case and that's very bug prone.
> For example, shouldn't we release f2fs_lock_op(sbi) when f2fs_add_link()
> fails earlier?

The pair of f2fs_lock_op and f2fs_unlock_op here is used to keep FS
consistency.
And, the handle_failed_inode() should be covered by f2fs_lock_op, so there is
no reason to do unlock and lock redundantly to handle the error case.

Thanks,

> 
> Sometimes it does one thing which is good, but the name is just lazy.
> 
> -out:
> +unlock:
> 	mutext_unlock();
> 	return err;
> 
> Btw shouldn't we earlier if page_symlink() fails?
> 
> regards,
> dan carpenter

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: f2fs crypto: add symlink encryption

[Dan Carpenter]

On Tue, May 12, 2015 at 11:17:16AM -0700, Jaegeuk Kim wrote:
> > For example, shouldn't we release f2fs_lock_op(sbi) when f2fs_add_link()
> > fails earlier?
> 
> The pair of f2fs_lock_op and f2fs_unlock_op here is used to keep FS
> consistency.
> And, the handle_failed_inode() should be covered by f2fs_lock_op, so there is
> no reason to do unlock and lock redundantly to handle the error case.

Yes, you are right, but the code is still not correct unfortunately.

fs/f2fs/namei.c
   424  
   425          f2fs_lock_op(sbi);
   426          err = f2fs_add_link(dentry, inode);
   427          if (err)
   428                  goto out;
                        ^^^^^^^^
Holding the lock.  This is correct as you say.

   429          f2fs_unlock_op(sbi);
   430  
   431          if (f2fs_encrypted_inode(dir)) {
   432                  struct qstr istr = QSTR_INIT(symname, len);
   433  
   434                  err = f2fs_inherit_context(dir, inode, NULL);
   435                  if (err)
   436                          goto out;
                                ^^^^^^^^
Not holding the lock.  This is a double unlock bug.

   437  
   438                  err = f2fs_setup_fname_crypto(inode);
   439                  if (err)
   440                          goto out;

regards,
dan carpenter


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: f2fs crypto: add symlink encryption

[Jaegeuk Kim]

On Tue, May 12, 2015 at 09:36:21PM +0300, Dan Carpenter wrote:
> On Tue, May 12, 2015 at 11:17:16AM -0700, Jaegeuk Kim wrote:
> > > For example, shouldn't we release f2fs_lock_op(sbi) when f2fs_add_link()
> > > fails earlier?
> > 
> > The pair of f2fs_lock_op and f2fs_unlock_op here is used to keep FS
> > consistency.
> > And, the handle_failed_inode() should be covered by f2fs_lock_op, so there is
> > no reason to do unlock and lock redundantly to handle the error case.
> 
> Yes, you are right, but the code is still not correct unfortunately.

Yeah, it was. Please check the V2 patch.

Thanks,

> 
> fs/f2fs/namei.c
>    424  
>    425          f2fs_lock_op(sbi);
>    426          err = f2fs_add_link(dentry, inode);
>    427          if (err)
>    428                  goto out;
>                         ^^^^^^^^
> Holding the lock.  This is correct as you say.
> 
>    429          f2fs_unlock_op(sbi);
>    430  
>    431          if (f2fs_encrypted_inode(dir)) {
>    432                  struct qstr istr = QSTR_INIT(symname, len);
>    433  
>    434                  err = f2fs_inherit_context(dir, inode, NULL);
>    435                  if (err)
>    436                          goto out;
>                                 ^^^^^^^^
> Not holding the lock.  This is a double unlock bug.
> 
>    437  
>    438                  err = f2fs_setup_fname_crypto(inode);
>    439                  if (err)
>    440                          goto out;
> 
> regards,
> dan carpenter

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y