Re: f2fs: avoid abnormal behavior on broken symlink

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

On Wed, Apr 22, 2015 at 11:13:36AM -0700, Jaegeuk Kim wrote:
> On Wed, Apr 22, 2015 at 01:27:22PM +0300, Dan Carpenter wrote:
> > On Wed, Apr 22, 2015 at 02:28:22PM +0800, Chao Yu wrote:
> > > > > fs/f2fs/namei.c
> > > > >    299  static void *f2fs_follow_link(struct dentry *dentry, struct nameidata *nd)
> > > > >    300  {
> > > > >    301          struct page *page;
> > > > >    302
> > > > >    303          page = page_follow_link_light(dentry, nd);
> > > > >    304          if (IS_ERR(page))
> > > > >                            ^^^^
> > > > > The code in page_follow_link_light() is a bit hard to follow but it
> > > > > returns NULL on error.
> > > 
> > > I try to find out the other callers' error handling method for
> > > page_follow_link_light, and it shows all of them use the "IS_ERR" one.
> > > 
> > > In page_follow_link_light I also can't find a path which will return a NULL value.
> > > 
> > > So, Dan, is that report from smatch not true? or I made a mistake? If so, please
> > > correct me.
> > 
> > Smatch is correct, it returns NULL on error.  However, I agree that it
> > looks like it is supposed to return an ERR_PTR.  Every caller seems to
> > expect that.
> 
> Well, at least, it seems vfs handles the error correctly.
> The page_follow_link_light is used only by several filesystems.
> 
> And, follow_link is called by:
> 
> 1. fs/namei.c <<follow_link>>
>  -> No problem. It checks the error with nd->saved_names.
> 
> 2. fs/namei.c <<generic_readlink>>
>  -> No problem. The error is checked by readlink_copy.

You're right.  My patch was wrong because it doesn't set
nd->saved_names.  But wow, though!  So convoluted!

  4423  /*
  4424   * A helper for ->readlink().  This should be used *ONLY* for symlinks that
  4425   * have ->follow_link() touching nd only in nd_set_link().  Using (or not
  4426   * using) it for any given inode is up to filesystem.
  4427   */
  4428  int generic_readlink(struct dentry *dentry, char __user *buffer, int buflen)
  4429  {
  4430          struct nameidata nd;
  4431          void *cookie;
  4432          int res;
  4433  
  4434          nd.depth = 0;
  4435          cookie = dentry->d_inode->i_op->follow_link(dentry, &nd);
  4436          if (IS_ERR(cookie))
  4437                  return PTR_ERR(cookie);

Even though we don't return here, the rest of the function is a no-op.

  4438  
  4439          res = readlink_copy(buffer, buflen, nd_get_link(&nd));
  4440          if (dentry->d_inode->i_op->put_link)
  4441                  dentry->d_inode->i_op->put_link(dentry, &nd, cookie);
  4442          return res;
  4443  }
  4444  EXPORT_SYMBOL(generic_readlink);

> 
> Whatever it is expected or not, the fact is that there is no bug in vfs.
> So at this moment, IMO, it needs to fix f2fs_follow_link by adding
> IS_ERR_OR_NULL() to avoid all the cases (including the backporting effort).
> 

I guess that works...  The follow_link() functions often seem to return
NULL on sucess.  But the page_follow_link_light() only returns NULL on
error.  It's very strange.

regards,
dan carpenter


------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
Develop your own process in accordance with the BPMN 2 standard
Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF