[Cluster-devel] [bug report] gfs2: Move inode generation number check into gfs2_inode_lookup

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

Hello Andreas Gruenbacher,

The patch b66648ad6dcf: "gfs2: Move inode generation number check
into gfs2_inode_lookup" from Jan 15, 2020, leads to the following
static checker warning:

	fs/gfs2/inode.c:227 gfs2_inode_lookup()
	warn: passing zero to 'ERR_PTR'

fs/gfs2/inode.c
   112   * If @type is DT_UNKNOWN, the inode type is fetched from disk.
   113   *
   114   * If @blktype is anything other than GFS2_BLKST_FREE (which is used as a
   115   * placeholder because it doesn't otherwise make sense), the on-disk block type
   116   * is verified to be @blktype.
   117   *
   118   * When @no_formal_ino is non-zero, this function will return ERR_PTR(-ESTALE)
   119   * if it detects that @no_formal_ino doesn't match the actual inode generation
   120   * number.  However, it doesn't always know unless @type is DT_UNKNOWN.
   121   *
   122   * Returns: A VFS inode, or an error
                    ^^^^^^^^^^^^^^^^^^^^^^^^
The comments imply this does not return NULL.

   123   */
   124  
   125  struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
   126                                  u64 no_addr, u64 no_formal_ino,
   127                                  unsigned int blktype)
   128  {
   129          struct inode *inode;
   130          struct gfs2_inode *ip;
   131          struct gfs2_glock *io_gl = NULL;
   132          struct gfs2_holder i_gh;
   133          int error;
   134  
   135          gfs2_holder_mark_uninitialized(&i_gh);
   136          inode = gfs2_iget(sb, no_addr);
   137          if (!inode)
   138                  return ERR_PTR(-ENOMEM);
   139  
   140          ip = GFS2_I(inode);
   141  
   142          if (inode->i_state & I_NEW) {
                    ^^^^^^^^^^^^^^^^^^^^^^
We take this path.

   143                  struct gfs2_sbd *sdp = GFS2_SB(inode);
   144  
   145                  error = gfs2_glock_get(sdp, no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl);
   146                  if (unlikely(error))
   147                          goto fail;
   148                  flush_delayed_work(&ip->i_gl->gl_work);
   149  
   150                  error = gfs2_glock_get(sdp, no_addr, &gfs2_iopen_glops, CREATE, &io_gl);
   151                  if (unlikely(error))
   152                          goto fail;
   153  
   154                  if (type == DT_UNKNOWN || blktype != GFS2_BLKST_FREE) {
   155                          /*
   156                           * The GL_SKIP flag indicates to skip reading the inode
   157                           * block.  We read the inode with gfs2_inode_refresh
   158                           * after possibly checking the block type.
   159                           */
   160                          error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE,
   161                                                     GL_SKIP, &i_gh);
   162                          if (error)
   163                                  goto fail;
   164  
   165                          error = -ESTALE;
   166                          if (no_formal_ino &&
   167                              gfs2_inode_already_deleted(ip->i_gl, no_formal_ino))
   168                                  goto fail;
   169  
   170                          if (blktype != GFS2_BLKST_FREE) {
   171                                  error = gfs2_check_blk_type(sdp, no_addr,
   172                                                              blktype);
   173                                  if (error)
   174                                          goto fail;
   175                          }
   176                  }
   177  
   178                  glock_set_object(ip->i_gl, ip);
   179                  set_bit(GIF_INVALID, &ip->i_flags);
   180                  error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, GL_EXACT, &ip->i_iopen_gh);
   181                  if (unlikely(error))
   182                          goto fail;
   183                  gfs2_cancel_delete_work(ip->i_iopen_gh.gh_gl);
   184                  glock_set_object(ip->i_iopen_gh.gh_gl, ip);
   185                  gfs2_glock_put(io_gl);
   186                  io_gl = NULL;
   187  
   188                  /* Lowest possible timestamp; will be overwritten in gfs2_dinode_in. */
   189                  inode->i_atime.tv_sec = 1LL << (8 * sizeof(inode->i_atime.tv_sec) - 1);
   190                  inode->i_atime.tv_nsec = 0;
   191  
   192                  if (type == DT_UNKNOWN) {
   193                          /* Inode glock must be locked already */
   194                          error = gfs2_inode_refresh(GFS2_I(inode));
   195                          if (error)
   196                                  goto fail;
   197                  } else {
   198                          ip->i_no_formal_ino = no_formal_ino;
   199                          inode->i_mode = DT2IF(type);
   200                  }
   201  
   202                  if (gfs2_holder_initialized(&i_gh))
   203                          gfs2_glock_dq_uninit(&i_gh);
   204  
   205                  gfs2_set_iop(inode);
   206          }
   207  
   208          if (no_formal_ino && ip->i_no_formal_ino &&
   209              no_formal_ino != ip->i_no_formal_ino) {
   210                  if (inode->i_state & I_NEW)
   211                          goto fail;
                                ^^^^^^^^^
"error" is zero here.

   212                  iput(inode);
   213                  return ERR_PTR(-ESTALE);
   214          }
   215  
   216          if (inode->i_state & I_NEW)
   217                  unlock_new_inode(inode);
   218  
   219          return inode;
   220  
   221  fail:
   222          if (io_gl)
   223                  gfs2_glock_put(io_gl);
   224          if (gfs2_holder_initialized(&i_gh))
   225                  gfs2_glock_dq_uninit(&i_gh);
   226          iget_failed(inode);
   227          return ERR_PTR(error);
                               ^^^^^
Leading to a NULL return.  It doesn't look like the caller checks for
NULL so it might lead to an Oops.

   228  }

regards,
dan carpenter