Re: [PATCH] ceph: Fix error handling in fill_readdir_cache()

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Wed, Mar 05, 2025 at 04:29:44AM +0000, Matthew Wilcox wrote:
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index c15970fa240f..6ac2bd555e86 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -1870,9 +1870,12 @@ static int fill_readdir_cache(struct inode *dir, struct dentry *dn,
> > >  
> > >  		ctl->folio = __filemap_get_folio(&dir->i_data, pgoff,
> > >  				fgf, mapping_gfp_mask(&dir->i_data));
> > 
> > Could we expect to receive NULL here somehow? I assume we should receive valid
> > pointer or ERR_PTR always here.
> 
> There's no way to get a NULL pointer here.  __filemap_get_folio() always
> returns a valid folio or an ERR_PTR.
> 
> > > -		if (!ctl->folio) {
> > > +		if (IS_ERR(ctl->folio)) {
> > > +			int err = PTR_ERR(ctl->folio);
> > > +
> > > +			ctl->folio = NULL;
> > >  			ctl->index = -1;
> > > -			return idx == 0 ? -ENOMEM : 0;
> > > +			return idx == 0 ? err : 0;
> > >  		}
> > >  		/* reading/filling the cache are serialized by
> > >  		 * i_rwsem, no need to use folio lock */
> > 
> > But I prefer to check on NULL anyway, because we try to unlock the folio here:
> > 
> > 		/* reading/filling the cache are serialized by
> > 		 * i_rwsem, no need to use folio lock */
> > 		folio_unlock(ctl->folio);
> > 
> > And absence of check on NULL makes me slightly nervous. :)
> 
> We'd get a very visible and obvious splat if we did!  But we make this
> assumption all over the VFS and in other filesystems.  There's no need
> to be more cautious in ceph than in other places.

Yeah.  When a function returns both error pointers and NULL that is
a specific thing.

https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

Adding pointless NULL checks is confusing and only leads to philosophical
debates about whether the future code which adds a NULL is more likely to
be a success path or a failure path.  There is no good answer.

I have a static checker warning for when people accidentally check for
IS_ERR() instead of NULL or when a function can return both but we
only check for error pointers.  But it turns out that I needed to update
it and also I hadn't published it.  I'll test the updated version
tonight and publish it later this week.  Attached.

regards,
dan carpenter

/*
 * Copyright (C) 2018 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_slist.h"
#include "smatch_extra.h"

static int my_id;

STATE(null);

static void deref_hook(struct expression *expr)
{
	struct smatch_state *estate;
	struct sm_state *sm;
	char *name;

	sm = get_sm_state_expr(my_id, expr);
	if (!sm || !slist_has_state(sm->possible, &null))
		return;
	if (implied_not_equal(expr, 0))
		return;
	estate = get_state_expr(SMATCH_EXTRA, expr);
	if (estate_is_empty(estate))
		return;

	name = expr_to_str(expr);
	sm_msg("warn: '%s' can also be NULL", name);
	free_string(name);
}

static void match_condition(struct expression *expr)
{
	struct data_range *drange;
	struct expression *arg;
	struct sm_state *sm, *tmp;

	expr = strip_expr(expr);
	if (expr->type != EXPR_CALL)
		return;
	if (!sym_name_is("IS_ERR", expr->fn))
		return;

	arg = get_argument_from_call_expr(expr->args, 0);
	arg = strip_expr(arg);

	if (!arg || implied_not_equal(arg, 0))
		return;

	sm = get_sm_state_expr(SMATCH_EXTRA, arg);
	if (!sm)
		return;

	FOR_EACH_PTR(sm->possible, tmp) {
		if (!estate_rl(tmp->state))
			continue;
		drange = first_ptr_list((struct ptr_list *)estate_rl(tmp->state));
		if (drange->min.value == 0 && drange->max.value == 0)
			goto has_null;
	} END_FOR_EACH_PTR(tmp);

	return;

has_null:
	set_true_false_states_expr(my_id, arg, NULL, &null);
}

void check_no_null_check_on_mixed(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_hook(&match_condition, CONDITION_HOOK);
	add_modification_hook(my_id, &set_undefined);
	add_dereference_hook(&deref_hook);
}