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 17:23:49 -0400
From:   Phil Auld <pauld@redhat.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: <YtCJZT08lHrsLul7@lorien.usersys.redhat.com>
References: <20220714183021.2924119-1-pauld@redhat.com>
 <YtBkA3QvP8JDGbhX@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YtBkA3QvP8JDGbhX@kroah.com>
Precedence: bulk

Hi Greg,

On Thu, Jul 14, 2022 at 08:44:19PM +0200 Greg Kroah-Hartman wrote:
> 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?
>

My git doesn't do that. Maybe I'm missing a useful config.

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

Fixed.

> 
> > 
> > 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?
>

Will do.


> > --- 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...
>

Aargh, yes, sorry. I used to use the RH internal version all the time but the workflow
has changed and it's not needed anymore. Totally forget about the uptream version.
You'd think this was my first upstream patch...

I'll fix up the numbers and clean it up for the next version.


Cheers,
Phil


> greg k-h
> 

--