From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Date: Tue, 9 Jun 2020 15:05:55 +0300 Subject: [Cluster-devel] [bug report] gfs2: Move inode generation number check into gfs2_inode_lookup Message-ID: <20200609120555.GA52680@mwanda> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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