Re: [PTACH] tmpfs: Fix undefined-behaviour in shmem_reconfigure()

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 13 May 2022 10:52:25 +0800 Luo Meng <luomeng12@huawei.com> wrote:

> When shmem_reconfigure() calls __percpu_counter_compare(), the second
> parameter is unsigned int.

Actually unsigned long long.

> But when define __percpu_counter_compare(),
> the second parameter is signed int.

Actually s64.

> So the abs of count - rhs, UBSAN shows the following warning:
> 

Here's my amended paragraph:

: When shmem_reconfigure() calls __percpu_counter_compare(), the second
: parameter is unsigned long long.  But in the definition of
: __percpu_counter_compare(), the second parameter is s64.  So when
: __percpu_counter_compare() executes abs(count - rhs), UBSAN shows the
: following warning:

> ...
> 
> Signed-off-by: Luo Meng <luomeng12@huawei.com>
> ---
>  mm/shmem.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 4b2fea33158e..15733c480318 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3487,6 +3487,10 @@ static int shmem_reconfigure(struct fs_context *fc)
>  
>  	raw_spin_lock(&sbinfo->stat_lock);
>  	inodes = sbinfo->max_inodes - sbinfo->free_inodes;
> +	if (ctx->blocks > S64_MAX) {
> +		err = "The number of blocks too large";

Should be "is too large".  But to match the other error messages'
style, we can use simply "Number of blocks too large".

> +		goto out;
> +	}
>  	if ((ctx->seen & SHMEM_SEEN_BLOCKS) && ctx->blocks) {
>  		if (!sbinfo->max_blocks) {
>  			err = "Cannot retroactively limit size";

I suppose that's a reasonable fix.  I prefer "get the darned types
right" but shmem_options.blocks should be unsigned (can't have a
negative block count) and changing percpu_counter() would be tough.

--- a/mm/shmem.c~tmpfs-fix-undefined-behaviour-in-shmem_reconfigure-fix
+++ a/mm/shmem.c
@@ -3477,7 +3477,7 @@ static int shmem_reconfigure(struct fs_c
 	raw_spin_lock(&sbinfo->stat_lock);
 	inodes = sbinfo->max_inodes - sbinfo->free_inodes;
 	if (ctx->blocks > S64_MAX) {
-		err = "The number of blocks too large";
+		err = "Number of blocks too large";
 		goto out;
 	}
 	if ((ctx->seen & SHMEM_SEEN_BLOCKS) && ctx->blocks) {
_