Re: f2fs crypto: add symlink encryption

[Dan Carpenter <dan.carpenter@oracle.com>]

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