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

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

[Matthew Wilcox (Oracle)]

__filemap_get_folio() returns an ERR_PTR, not NULL.  There are extensive
assumptions that ctl->folio is NULL, not an error pointer, so it seems
better to fix this one place rather than change all the places which
check ctl->folio.

Fixes: baff9740bc8f ("ceph: Convert ceph_readdir_cache_control to store a folio")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
---
 fs/ceph/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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));
-		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 */
-- 
2.47.2

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

[Viacheslav Dubeyko]

On Tue, 2025-03-04 at 15:48 +0000, Matthew Wilcox (Oracle) wrote:
> __filemap_get_folio() returns an ERR_PTR, not NULL.  There are extensive
> assumptions that ctl->folio is NULL, not an error pointer, so it seems
> better to fix this one place rather than change all the places which
> check ctl->folio.
> 
> Fixes: baff9740bc8f ("ceph: Convert ceph_readdir_cache_control to store a folio")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> ---
>  fs/ceph/inode.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 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.

> -		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. :)

Thanks,
Slava.

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

[Matthew Wilcox]

On Tue, Mar 04, 2025 at 06:41:46PM +0000, Viacheslav Dubeyko wrote:
> On Tue, 2025-03-04 at 15:48 +0000, Matthew Wilcox (Oracle) wrote:
> > __filemap_get_folio() returns an ERR_PTR, not NULL.  There are extensive
> > assumptions that ctl->folio is NULL, not an error pointer, so it seems
> > better to fix this one place rather than change all the places which
> > check ctl->folio.
> > 
> > Fixes: baff9740bc8f ("ceph: Convert ceph_readdir_cache_control to store a folio")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > ---
> >  fs/ceph/inode.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > 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.

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

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


[-- Attachment #2: check_no_null_check_on_mixed.c --]
[-- Type: text/x-csrc, Size: 2242 bytes --]

/*
 * 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);
}

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

[Christian Brauner]

On Tue, 04 Mar 2025 15:48:16 +0000, Matthew Wilcox (Oracle) wrote:
> __filemap_get_folio() returns an ERR_PTR, not NULL.  There are extensive
> assumptions that ctl->folio is NULL, not an error pointer, so it seems
> better to fix this one place rather than change all the places which
> check ctl->folio.
> 
> 

Applied to the vfs-6.15.ceph branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.ceph branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.ceph

[1/1] ceph: Fix error handling in fill_readdir_cache()
      https://git.kernel.org/vfs/vfs/c/efbdd92ed9f6