Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 20:44:19 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Phil Auld <pauld@redhat.com>
Cc:     linux-kernel@vger.kernel.org,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Barry Song <21cnbao@gmail.com>,
        Tian Tao <tiantao6@hisilicon.com>,
        Yury Norov <yury.norov@gmail.com>
Subject: Re: [PATCH v4 RESEND] drivers/base: fix userspace break from using
 bin_attributes for cpumap and cpulist
Message-ID: <YtBkA3QvP8JDGbhX@kroah.com>
References: <20220714183021.2924119-1-pauld@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220714183021.2924119-1-pauld@redhat.com>
Precedence: bulk

On Thu, Jul 14, 2022 at 02:30:21PM -0400, Phil Auld wrote:
> Using bin_attributes with a 0 size causes fstat and friends to return that 0 size.
> This breaks userspace code that retrieves the size before reading the file. Rather
> than reverting 75bd50fa841 ("drivers/base/node.c: use bin_attribute to break the size
> limitation of cpumap ABI") let's put in a size value at compile time. Use direct
> comparison and a worst-case maximum to ensure compile time constants. For cpulist the
> max is on the order of NR_CPUS * (ceil(log10(NR_CPUS)) + 1) which for 8192 is 40960
> (8192 * 5). In order to get near that you'd need a system with every other CPU on one
> node or something similar. e.g. (0,2,4,8, ... ). To simplify the math and support
> larger NR_CPUS in the future we are using NR_CPUS * 7. We also set it to a min of
> PAGE_SIZE to retain the older behavior for smaller NR_CPUS. The cpumap file wants to
> be something like NR_CPUS/4 + NR_CPUS/32, for the ","s so for simplicity we are using
> NR_CPUS/2.
> 
> Add a set of macros for these values to cpumask.h so they can be used in multiple places.
> Apply these to the handful of such files in drivers/base/topology.c as well as node.c.

Git should have asked you to round at 72 columns, right?

And that's one huge wall of text for the first paragraph, can you make
that more readable?

> 
> On an 80 cpu 4-node sytem (NR_CPUS == 8192)
> 
> before:
> 
> -r--r--r--. 1 root root 0 Jul 12 14:08 /sys/devices/system/node/node0/cpulist
> -r--r--r--. 1 root root 0 Jul 11 17:25 /sys/devices/system/node/node0/cpumap
> 
> after:
> 
> -r--r--r--. 1 root root 57344 Jul 13 11:32 /sys/devices/system/node/node0/cpulist
> -r--r--r--. 1 root root  4096 Jul 13 11:31 /sys/devices/system/node/node0/cpumap
> 
> CONFIG_NR_CPUS = 16384
> -r--r--r--. 1 root root 114688 Jul 13 14:03 /sys/devices/system/node/node0/cpulist
> -r--r--r--. 1 root root   8192 Jul 13 14:02 /sys/devices/system/node/node0/cpumap
> 
> Fixes: 75bd50fa841 ("drivers/base/node.c: use bin_attribute to break the size limitation of cpumap ABI")
> Fixes: bb9ec13d156 ("topology: use bin_attribute to break the size limitation of cpumap ABI")
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Phil Auld <pauld@redhat.com>

You want to cc: stable too, right?

> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -1071,4 +1071,20 @@ cpumap_print_list_to_buf(char *buf, const struct cpumask *mask,
>  	[0] =  1UL							\
>  } }
>  
> +/* 
> + * Provide a valid theoretical max size for cpumap ands cpulist sysfs files to 
> + * avoid breaking userspace which may allocate a buffer based on the size 
> + * reported by e.g. fstat.
> + *
> + * For cpumap NR_CPUS/2 is a simplification of NR_CPUS/4 + NR_CPUS/32. 
> + *
> + * For cpulist 7 is (ceil(log10(NR_CPUS)) + 1) allowing for NR_CPUS to be up to 
> + * 2 orders of magnitude larger than 8192. This covers a worst-case of every 
> + * other cpu being on one of two nodes for a very large NR_CPUS.
> + *
> + *  Use PAGE_SIZE as a minimum for smaller configurations. 
> + */

Please run checkpatch on your patches before sending them out and
getting grumpy emails from maintainers asking you to run checkpatch on
your patches...

greg k-h