ufs: Handle NULL return from ufs_get_locked_folio()

[PATCH] ufs: Handle NULL return from ufs_get_locked_folio()

Posted by Ingyu Jang 4 weeks ago

ufs_get_locked_folio() may return either an error pointer (when
read_mapping_folio() fails) or NULL (when a concurrent truncate has
detached the folio from its mapping). The current IS_ERR() check in
ufs_alloc_lastblock() only handles the error-pointer case; a NULL
return falls through to folio_buffers(folio), causing a NULL pointer
dereference under the truncate race.

Use IS_ERR_OR_NULL() to cover both failure modes.

Signed-off-by: Ingyu Jang <ingyujang25@korea.ac.kr>
---
 fs/ufs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 440d014cc5ed5..989ff3d3ad179 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -1051,7 +1051,7 @@ static int ufs_alloc_lastblock(struct inode *inode, loff_t size)
 
 	folio = ufs_get_locked_folio(mapping, lastfrag >>
 				       (PAGE_SHIFT - inode->i_blkbits));
-	if (IS_ERR(folio)) {
+	if (IS_ERR_OR_NULL(folio)) {
 		err = -EIO;
 		goto out;
 	}
-- 
2.34.1

Re: [PATCH] ufs: Handle NULL return from ufs_get_locked_folio()

Posted by Jan Kara 3 weeks, 6 days ago

On Fri 15-05-26 04:32:56, Ingyu Jang wrote:
> ufs_get_locked_folio() may return either an error pointer (when
> read_mapping_folio() fails) or NULL (when a concurrent truncate has
> detached the folio from its mapping). The current IS_ERR() check in
> ufs_alloc_lastblock() only handles the error-pointer case; a NULL
> return falls through to folio_buffers(folio), causing a NULL pointer
> dereference under the truncate race.
> 
> Use IS_ERR_OR_NULL() to cover both failure modes.
> 
> Signed-off-by: Ingyu Jang <ingyujang25@korea.ac.kr>

Well, but this shouldn't really happen because ufs_alloc_lastblock() is
called only from ufs_truncate() where it is protected from folio being
detached by i_rwsem. Still probably makes sense from code robustness POV
though so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

but please update changelog to reflect this.

								Honza

> ---
>  fs/ufs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
> index 440d014cc5ed5..989ff3d3ad179 100644
> --- a/fs/ufs/inode.c
> +++ b/fs/ufs/inode.c
> @@ -1051,7 +1051,7 @@ static int ufs_alloc_lastblock(struct inode *inode, loff_t size)
>  
>  	folio = ufs_get_locked_folio(mapping, lastfrag >>
>  				       (PAGE_SHIFT - inode->i_blkbits));
> -	if (IS_ERR(folio)) {
> +	if (IS_ERR_OR_NULL(folio)) {
>  		err = -EIO;
>  		goto out;
>  	}
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2] ufs: Defensively handle NULL return from ufs_get_locked_folio()

Posted by Ingyu Jang 3 weeks, 3 days ago

ufs_get_locked_folio() may return either an error pointer (when
read_mapping_folio() fails) or NULL (when a concurrent truncate has
detached the folio from its mapping). In ufs_alloc_lastblock() the
NULL path cannot be reached in practice because the only caller,
ufs_truncate(), holds i_rwsem and so serialises against other
truncates. Still, the current IS_ERR()-only check does not match the
documented contract of ufs_get_locked_folio() and is fragile against
future callers.

Use IS_ERR_OR_NULL() to match the helper's return contract.

Signed-off-by: Ingyu Jang <ingyujang25@korea.ac.kr>
Reviewed-by: Jan Kara <jack@suse.cz>
---
Changes in v2:
- Rewrote changelog per Jan Kara's review feedback to clarify that the
  NULL path is unreachable in practice (i_rwsem in ufs_truncate()), and
  framed the change as matching the helper's return contract / future-
  proofing rather than fixing an active race.
- Diff unchanged.

Link to v1: https://lore.kernel.org/all/20260514193256.2556905-1-ingyujang25@korea.ac.kr/

 fs/ufs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index 440d014cc5ed5..989ff3d3ad179 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -1051,7 +1051,7 @@ static int ufs_alloc_lastblock(struct inode *inode, loff_t size)
 
 	folio = ufs_get_locked_folio(mapping, lastfrag >>
 				       (PAGE_SHIFT - inode->i_blkbits));
-	if (IS_ERR(folio)) {
+	if (IS_ERR_OR_NULL(folio)) {
 		err = -EIO;
 		goto out;
 	}
-- 
2.34.1