Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Damien Le Moal <dlemoal@kernel.org>]

On 8/12/25 12:32 PM, Rajeev Mishra wrote:
> Hi Kuai,
> 
> Thank you for the feedback on the v2 patch regarding error handling.
> 
> Yu mentioned:
>> return 0 here is odd. Why not "return ret;" to propagate the error if any ?
> 
> I understand the concern about proper error propagation. However, there's a 
> type compatibility issue I'd like to discuss before implementing v3:
> 
> 1. Current function signature: `static loff_t get_size(...)` 
>    - Returns size as positive loff_t (unsigned 64-bit)  
>    - All callers expect non-negative size values
> 
> 2. vfs_getattr_nosec() error codes are negative integers (-ENOENT, -EIO, etc.)
>    - Returning `ret` would cast negative errors to huge positive numbers
>    - This could cause loop devices to appear as exabyte-sized
> 
> 3. Current callers like loop_set_size() don't handle error checking
> 
> Would you prefer for v3:
> a) Change function signature to `int get_size(..., loff_t *size)` and update all callers  
> b) Different approach?
> 
> diff with ret approach
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c418c47db76e..15117630c6c1 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -142,12 +142,13 @@ static int part_shift;
>   * @offset: offset into the backing file
>   * @sizelimit: user-specified size limit
>   * @file: the backing file
> + * @size: pointer to store the calculated size
>   *
>   * Calculate the effective size of the loop device
>   *
> - * Returns: size in 512-byte sectors, or 0 if invalid
> + * Returns: 0 on success, negative error code on failure
>   */
> -static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)

Since loff_t is "long long", so a signed type, I would keep this interface and
add a negative error check in the 2 call sites for get_size(). That is simpler.

> +static int get_size(loff_t offset, loff_t sizelimit, struct file *file, loff_t *size)
>  {
>         struct kstat stat;
>         loff_t loopsize;
> @@ -159,7 +160,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>          */
>         ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
>         if (ret)
> -               return 0;
> +               return ret;
> 
>         loopsize = stat.size;
> 
> @@ -167,7 +168,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>                 loopsize -= offset;
>         /* offset is beyond i_size, weird but possible */
>         if (loopsize < 0)
> -               return 0;
> +               return -EINVAL;
> 
>         if (sizelimit > 0 && sizelimit < loopsize)
>                 loopsize = sizelimit;
> @@ -175,12 +176,20 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>          * Unfortunately, if we want to do I/O on the device,
>          * the number of 512-byte sectors has to fit into a sector_t.
>          */
> -       return loopsize >> 9;
> +       *size = loopsize >> 9;
> +       return 0;
>  }
> 
>  static loff_t get_loop_size(struct loop_device *lo, struct file *file)
>  {
> -       return get_size(lo->lo_offset, lo->lo_sizelimit, file);
> +       loff_t size;
> +       int ret;
> +
> +       ret = get_size(lo->lo_offset, lo->lo_sizelimit, file, &size);
> +       if (ret)
> +               return 0;  /* Fallback to 0 on error for backward compatibility */
> +
> +       return size;
>  }
> 
> 
> I am happy to implement whichever direction you think is best.
> 
> Thanks,
> Rajeev


-- 
Damien Le Moal
Western Digital Research