On Thu, Jan 12, 2023 at 03:43:31AM +0000, Al Viro wrote:
> On Thu, Jan 12, 2023 at 01:32:48AM +0000, Peng Zhang wrote:
> > From: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> >
> > Dan Carpenter reported a Smatch static checker warning:
> >
> > fs/ntfs3/namei.c:96 ntfs_lookup()
> > error: potential NULL/IS_ERR bug 'inode'
> > It will cause null-ptr-deref when dir_search_u() returns NULL if the
> > file is not found.
> > Fix this by replacing IS_ERR() with IS_ERR_OR_NULL() to add a check for
> > NULL.
>
> That's a bad approach - you are papering over bad calling conventions instead of
> fixing them.
>
> IS_ERR_OR_NULL is almost never the right tool. Occasionally there are valid
> cases for function possibly returning pointer/NULL/ERR_PTR(...); this is
> almost certainly not one of those.
>
> Incidentally, inodes with NULL ->i_op should never exist. _Any_ place that
> sets ->i_op to NULL is broken, plain and simple. A new instance of struct
> inode has ->i_op pointing to empty method table; it *is* initialized.
IOW, the real bug is in ntfs_read_mft() -
inode->i_op = NULL;
in there is garbage. Unless I'm misreading the history, it used to be possible
for the damn thing to get all the way to ntfs_lookup() - up until the
commit 0e8235d28f3a "fs/ntfs3: Check fields while reading" had taken that
path out:
- if (!is_rec_base(rec))
- goto Ok;
+ if (!is_rec_base(rec)) {
+ err = -EINVAL;
+ goto out;
+ }
is the relevant part. Situation after that commit:
* useless check in ntfs_lookup() is a dead code; it should be
taken out, especially since it's broken.
* NULL assignment in ntfs_read_mft() is still garbage; thankfully,
with the current tree the inode will either have it overwritten by later
assignment or it won't make it out of ntfs_read_mft(). Still, that
assignment should be taken out and shot to get rid of bad example.
While we are at it, the calling conventions of ntfs_read_mft() could've
been better. Look:
ntfs_read_mft(inode, ...) either returns its first argument (on success) or
it disposes of the inode the argument points to and returns ERR_PTR(-E...)
(on failure). There is only one caller, and it would be easier to follow if
it had been
/* If this is a freshly allocated inode, need to read it now. */
if (inode->i_state & I_NEW) {
int err = ntfs_read_mft(inode, name, ref);
if (unlikely(err)) {
if (name)
ntfs_set_state(sb->s_fs_info, NTFS_DIRTY_ERROR);
iget_failed(inode);
return ERR_PTR(err);
}
} else if (ref->seq != ntfs_i(inode)->mi.mrec->seq) {
/* Inode overlaps? */
_ntfs_bad_inode(inode);
}
return inode;
with ntfs_read_mft() always acting the same way wrt inode refcount...
