| Message ID | 20250304154818.250757-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: Fix error handling in fill_readdir_cache() | expand |
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.
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.
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
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
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 */
__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(-)