linux-kernel - Re: [PATCH v2] fs: inode: return proper error code in bmap()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 16 Jul 2023 09:36:41 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Leesoo Ahn <lsahn@...eel.net>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Leesoo Ahn <lsahn@...akecorp.com>
Subject: Re: [PATCH v2] fs: inode: return proper error code in bmap()

On Sat, Jul 15, 2023 at 05:22:04PM +0900, Leesoo Ahn wrote:
> Return -EOPNOTSUPP instead of -EINVAL which has the meaning of
> the argument is an inappropriate value. The current error code doesn't
> make sense to represent that a file system doesn't support bmap operation.
> 
> Signed-off-by: Leesoo Ahn <lsahn@...akecorp.com>
> ---
> Changes since v1:
> - Modify the comments of bmap()
> - Modify subject and description requested by Markus Elfring
> https://lore.kernel.org/lkml/20230715060217.1469690-1-lsahn@wewakecorp.com/
> 
>  fs/inode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 8fefb69e1f84..697c51ed226a 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1831,13 +1831,13 @@ EXPORT_SYMBOL(iput);
>   *	4 in ``*block``, with disk block relative to the disk start that holds that
>   *	block of the file.
>   *
> - *	Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
> + *	Returns -EOPNOTSUPP in case of error, 0 otherwise. If mapping falls into a
>   *	hole, returns 0 and ``*block`` is also set to 0.
>   */
>  int bmap(struct inode *inode, sector_t *block)
>  {
>  	if (!inode->i_mapping->a_ops->bmap)
> -		return -EINVAL;
> +		return -EOPNOTSUPP;
>  
>  	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
>  	return 0;

What about the CONFIG_BLOCK=n wrapper?

Also, all the in kernel consumers squash this error back to 0, -EIO
or -EINVAL, so this change only ever propagates out to userspace via
the return from ioctl(FIBMAP).  Do we really need to change this and
risk breaking userspace that handles -EINVAL correctly but not
-EOPNOTSUPP?

-Dave.
-- 
Dave Chinner
david@...morbit.com