Dan Carpenter
2017-May-20 09:48 UTC
[Ocfs2-devel] [bug report] ocfs2: check/fix inode block for online file check
Hello Gang He,
The patch d56a8f32e4c6: "ocfs2: check/fix inode block for online file
check" from Mar 22, 2016, leads to the following static checker
warning:
fs/ocfs2/inode.c:179 ocfs2_iget()
warn: passing zero to 'ERR_PTR'
fs/ocfs2/inode.c
137 int sysfile_type)
138 {
139 int rc = 0;
^^^^^^
rc is zero.
140 struct inode *inode = NULL;
141 struct super_block *sb = osb->sb;
142 struct ocfs2_find_inode_args args;
143 journal_t *journal = OCFS2_SB(sb)->journal->j_journal;
144
145 trace_ocfs2_iget_begin((unsigned long long)blkno, flags,
146 sysfile_type);
147
148 /* Ok. By now we've either got the offsets passed to us by
the
149 * caller, or we just pulled them off the bh. Lets do some
150 * sanity checks to make sure they're OK. */
151 if (blkno == 0) {
152 inode = ERR_PTR(-EINVAL);
153 mlog_errno(PTR_ERR(inode));
154 goto bail;
155 }
156
157 args.fi_blkno = blkno;
158 args.fi_flags = flags;
159 args.fi_ino = ino_from_blkno(sb, blkno);
160 args.fi_sysfile_type = sysfile_type;
161
162 inode = iget5_locked(sb, args.fi_ino, ocfs2_find_actor,
163 ocfs2_init_locked_inode, &args);
164 /* inode was *not* in the inode cache. 2.6.x requires
165 * us to do our own read_inode call and unlock it
166 * afterwards. */
167 if (inode == NULL) {
168 inode = ERR_PTR(-ENOMEM);
169 mlog_errno(PTR_ERR(inode));
170 goto bail;
171 }
172 trace_ocfs2_iget5_locked(inode->i_state);
173 if (inode->i_state & I_NEW) {
174 rc = ocfs2_read_locked_inode(inode, &args);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
or it's set here
175 unlock_new_inode(inode);
176 }
177 if (is_bad_inode(inode)) {
178 iput(inode);
179 inode = ERR_PTR(rc);
^^^^^^^^^^^^^^^^^^^
then we possibly set inode to NULL. The callers I looked at are not
expecting so it will result in a NULL deref.
180 goto bail;
181 }
regards,
dan carpenter
Gang He
2017-May-22 03:20 UTC
[Ocfs2-devel] [bug report] ocfs2: check/fix inode block for online file check
Hello Dan, Thank for your reporting. I will send the patch for fixing this warning. Thanks Gang>>> > Hello Gang He, > > The patch d56a8f32e4c6: "ocfs2: check/fix inode block for online file > check" from Mar 22, 2016, leads to the following static checker > warning: > > fs/ocfs2/inode.c:179 ocfs2_iget() > warn: passing zero to 'ERR_PTR' > > fs/ocfs2/inode.c > 137 int sysfile_type) > 138 { > 139 int rc = 0; > ^^^^^^ > rc is zero. > > 140 struct inode *inode = NULL; > 141 struct super_block *sb = osb->sb; > 142 struct ocfs2_find_inode_args args; > 143 journal_t *journal = OCFS2_SB(sb)->journal->j_journal; > 144 > 145 trace_ocfs2_iget_begin((unsigned long long)blkno, flags, > 146 sysfile_type); > 147 > 148 /* Ok. By now we've either got the offsets passed to us by > the > 149 * caller, or we just pulled them off the bh. Lets do some > 150 * sanity checks to make sure they're OK. */ > 151 if (blkno == 0) { > 152 inode = ERR_PTR(-EINVAL); > 153 mlog_errno(PTR_ERR(inode)); > 154 goto bail; > 155 } > 156 > 157 args.fi_blkno = blkno; > 158 args.fi_flags = flags; > 159 args.fi_ino = ino_from_blkno(sb, blkno); > 160 args.fi_sysfile_type = sysfile_type; > 161 > 162 inode = iget5_locked(sb, args.fi_ino, ocfs2_find_actor, > 163 ocfs2_init_locked_inode, &args); > 164 /* inode was *not* in the inode cache. 2.6.x requires > 165 * us to do our own read_inode call and unlock it > 166 * afterwards. */ > 167 if (inode == NULL) { > 168 inode = ERR_PTR(-ENOMEM); > 169 mlog_errno(PTR_ERR(inode)); > 170 goto bail; > 171 } > 172 trace_ocfs2_iget5_locked(inode->i_state); > 173 if (inode->i_state & I_NEW) { > 174 rc = ocfs2_read_locked_inode(inode, &args); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > or it's set here > > 175 unlock_new_inode(inode); > 176 } > 177 if (is_bad_inode(inode)) { > 178 iput(inode); > 179 inode = ERR_PTR(rc); > ^^^^^^^^^^^^^^^^^^^ > then we possibly set inode to NULL. The callers I looked at are not > expecting so it will result in a NULL deref. > > 180 goto bail; > 181 } > > regards, > dan carpenter
Gang He
2017-May-23 02:20 UTC
[Ocfs2-devel] [bug report] ocfs2: check/fix inode block for online file check
Hi Dan, What static checker tool did you use? could you paste the detailed warning message to the list too? Since I need to write this information into the patch comments due to Andrew Morton's suggestion. Thanks a lot. Gang>>> > Hello Gang He, > > The patch d56a8f32e4c6: "ocfs2: check/fix inode block for online file > check" from Mar 22, 2016, leads to the following static checker > warning: > > fs/ocfs2/inode.c:179 ocfs2_iget() > warn: passing zero to 'ERR_PTR' > > fs/ocfs2/inode.c > 137 int sysfile_type) > 138 { > 139 int rc = 0; > ^^^^^^ > rc is zero. > > 140 struct inode *inode = NULL; > 141 struct super_block *sb = osb->sb; > 142 struct ocfs2_find_inode_args args; > 143 journal_t *journal = OCFS2_SB(sb)->journal->j_journal; > 144 > 145 trace_ocfs2_iget_begin((unsigned long long)blkno, flags, > 146 sysfile_type); > 147 > 148 /* Ok. By now we've either got the offsets passed to us by > the > 149 * caller, or we just pulled them off the bh. Lets do some > 150 * sanity checks to make sure they're OK. */ > 151 if (blkno == 0) { > 152 inode = ERR_PTR(-EINVAL); > 153 mlog_errno(PTR_ERR(inode)); > 154 goto bail; > 155 } > 156 > 157 args.fi_blkno = blkno; > 158 args.fi_flags = flags; > 159 args.fi_ino = ino_from_blkno(sb, blkno); > 160 args.fi_sysfile_type = sysfile_type; > 161 > 162 inode = iget5_locked(sb, args.fi_ino, ocfs2_find_actor, > 163 ocfs2_init_locked_inode, &args); > 164 /* inode was *not* in the inode cache. 2.6.x requires > 165 * us to do our own read_inode call and unlock it > 166 * afterwards. */ > 167 if (inode == NULL) { > 168 inode = ERR_PTR(-ENOMEM); > 169 mlog_errno(PTR_ERR(inode)); > 170 goto bail; > 171 } > 172 trace_ocfs2_iget5_locked(inode->i_state); > 173 if (inode->i_state & I_NEW) { > 174 rc = ocfs2_read_locked_inode(inode, &args); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > or it's set here > > 175 unlock_new_inode(inode); > 176 } > 177 if (is_bad_inode(inode)) { > 178 iput(inode); > 179 inode = ERR_PTR(rc); > ^^^^^^^^^^^^^^^^^^^ > then we possibly set inode to NULL. The callers I looked at are not > expecting so it will result in a NULL deref. > > 180 goto bail; > 181 } > > regards, > dan carpenter