* [Cluster-devel] [bug report] gfs2: Move inode generation number check into gfs2_inode_lookup
@ 2020-06-09 12:05 Dan Carpenter
2020-06-09 12:43 ` Andreas Gruenbacher
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-06-09 12:05 UTC (permalink / raw)
To: cluster-devel.redhat.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
^ permalink raw reply [flat|nested] 2+ messages in thread
* [Cluster-devel] [bug report] gfs2: Move inode generation number check into gfs2_inode_lookup
2020-06-09 12:05 [Cluster-devel] [bug report] gfs2: Move inode generation number check into gfs2_inode_lookup Dan Carpenter
@ 2020-06-09 12:43 ` Andreas Gruenbacher
0 siblings, 0 replies; 2+ messages in thread
From: Andreas Gruenbacher @ 2020-06-09 12:43 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi Dan,
On Tue, Jun 9, 2020 at 2:06 PM Dan Carpenter <dan.carpenter@oracle.com> 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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-09 12:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09 12:05 [Cluster-devel] [bug report] gfs2: Move inode generation number check into gfs2_inode_lookup Dan Carpenter
2020-06-09 12:43 ` Andreas Gruenbacher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.