[2/2] iomap: bmap: Remove the WARN and return the proper block address
diff mbox series

Message ID e2e09c5d840458b4ace6f9b31429ceefd9c1df01.1587670914.git.riteshh@linux.ibm.com
State New
Headers show
Series
  • WARN fibmap user in case of possible addr truncation
Related show

Commit Message

Ritesh Harjani April 24, 2020, 7:22 a.m. UTC
iomap_bmap() could be called from either of these two paths.
Either when a user is calling an ioctl_fibmap() interface to get
the block mapping address or by some filesystem via use of bmap()
internal kernel API.
bmap() kernel API is well equipped with handling of u64 addresses.

WARN condition in iomap_bmap_actor() was mainly added to warn all
the fibmap users. But now that in previous patch we have directly added
this WARN condition for all fibmap users and also made sure to return 0
as block map address in case if addr > INT_MAX. 
So we can now remove this logic from here.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/iomap/fiemap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Jan Kara April 24, 2020, 9:59 a.m. UTC | #1
On Fri 24-04-20 12:52:18, Ritesh Harjani wrote:
> iomap_bmap() could be called from either of these two paths.
> Either when a user is calling an ioctl_fibmap() interface to get
> the block mapping address or by some filesystem via use of bmap()
> internal kernel API.
> bmap() kernel API is well equipped with handling of u64 addresses.
> 
> WARN condition in iomap_bmap_actor() was mainly added to warn all
> the fibmap users. But now that in previous patch we have directly added
> this WARN condition for all fibmap users and also made sure to return 0
> as block map address in case if addr > INT_MAX. 
> So we can now remove this logic from here.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Yes, I agree it's better to hadle the overflow in the ioctl than in the iomap
actor. The patch looks good to me. You can add:

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

								Honza

> ---
>  fs/iomap/fiemap.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index bccf305ea9ce..d55e8f491a5e 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
>  
>  	if (iomap->type == IOMAP_MAPPED) {
>  		addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
> -		if (addr > INT_MAX)
> -			WARN(1, "would truncate bmap result\n");
> -		else
> -			*bno = addr;
> +		*bno = addr;
>  	}
>  	return 0;
>  }
> -- 
> 2.21.0
>
Christoph Hellwig April 24, 2020, 10:07 a.m. UTC | #2
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong April 24, 2020, 5:48 p.m. UTC | #3
On Fri, Apr 24, 2020 at 12:52:18PM +0530, Ritesh Harjani wrote:
> iomap_bmap() could be called from either of these two paths.
> Either when a user is calling an ioctl_fibmap() interface to get
> the block mapping address or by some filesystem via use of bmap()
> internal kernel API.
> bmap() kernel API is well equipped with handling of u64 addresses.
> 
> WARN condition in iomap_bmap_actor() was mainly added to warn all
> the fibmap users. But now that in previous patch we have directly added
> this WARN condition for all fibmap users and also made sure to return 0
> as block map address in case if addr > INT_MAX. 
> So we can now remove this logic from here.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  fs/iomap/fiemap.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index bccf305ea9ce..d55e8f491a5e 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
>  
>  	if (iomap->type == IOMAP_MAPPED) {
>  		addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
> -		if (addr > INT_MAX)
> -			WARN(1, "would truncate bmap result\n");

Frankly I would've combined these two patches to make it more obvious
that we're hoisting a FIBMAP constraint check from iomap into the ioctl
handler.

--D

> -		else
> -			*bno = addr;
> +		*bno = addr;
>  	}
>  	return 0;
>  }
> -- 
> 2.21.0
>
Ritesh Harjani April 24, 2020, 10:39 p.m. UTC | #4
On 4/24/20 11:18 PM, Darrick J. Wong wrote:
> On Fri, Apr 24, 2020 at 12:52:18PM +0530, Ritesh Harjani wrote:
>> iomap_bmap() could be called from either of these two paths.
>> Either when a user is calling an ioctl_fibmap() interface to get
>> the block mapping address or by some filesystem via use of bmap()
>> internal kernel API.
>> bmap() kernel API is well equipped with handling of u64 addresses.
>>
>> WARN condition in iomap_bmap_actor() was mainly added to warn all
>> the fibmap users. But now that in previous patch we have directly added
>> this WARN condition for all fibmap users and also made sure to return 0
>> as block map address in case if addr > INT_MAX.
>> So we can now remove this logic from here.
>>
>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>> ---
>>   fs/iomap/fiemap.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
>> index bccf305ea9ce..d55e8f491a5e 100644
>> --- a/fs/iomap/fiemap.c
>> +++ b/fs/iomap/fiemap.c
>> @@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
>>   
>>   	if (iomap->type == IOMAP_MAPPED) {
>>   		addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
>> -		if (addr > INT_MAX)
>> -			WARN(1, "would truncate bmap result\n");
> 
> Frankly I would've combined these two patches to make it more obvious
> that we're hoisting a FIBMAP constraint check from iomap into the ioctl
> handler.

Sure, let me combine the two in v2.

Thanks!!
-ritesh

> 
> --D
> 
>> -		else
>> -			*bno = addr;
>> +		*bno = addr;
>>   	}
>>   	return 0;
>>   }
>> -- 
>> 2.21.0
>>

Patch
diff mbox series

diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index bccf305ea9ce..d55e8f491a5e 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -117,10 +117,7 @@  iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
 
 	if (iomap->type == IOMAP_MAPPED) {
 		addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
-		if (addr > INT_MAX)
-			WARN(1, "would truncate bmap result\n");
-		else
-			*bno = addr;
+		*bno = addr;
 	}
 	return 0;
 }