Hi Dan, On Tue, Jun 9, 2020 at 2:06 PM Dan Carpenter <[email protected]> wrote: > 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.
Right, that's a bug. I've pushed a fix to https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next. Thanks, Andreas > 228 } > > regards, > dan carpenter
