On Fri, Dec 17, 2021 at 03:55:44PM -0800, Dave Hansen wrote:
> On 12/17/21 3:30 PM, Greg KH wrote:
> >>>
> >>> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> >>> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr 2021-12-17 13:38:00.217312383 -0800
> >>> +++ b/arch/x86/kernel/cpu/sgx/main.c 2021-12-17 14:00:36.293044390 -0800
> >>> @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >>> #ifdef CONFIG_NUMA
> >>> static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
> >>> {
> >>> - return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> >>> + unsigned long node_bytes = 0;
> >>> +
> >>> + /* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
> >>> + if (!nodes_empty(sgx_numa_mask))
> >>> + node_bytes = sgx_numa_nodes[dev->id].size;
> >>> +
> >>> + return sysfs_emit(buf, "%lu\n", node_bytes);
> >>> }
> >> Why is this file showing up if we do not have sgx_numa_nodes not
> >> allocated? It shouldn't even be there to access then.
> >>
> >> don't return a fake number, just don't present the sysfs file at all.
> > Or, if you _have_ to have the file present, return an error instead of a
> > fake value.
> >
> > But really, only create the file if the system supports it, that's the
> > rule for sysfs and it makes it very easy to do so (see the is_visible
> > callback for the attribute group for how to do it.)
>
> Thanks for the suggestion! That's a lot nicer. It's also dirt simple
> since we only have one attribute. Updated patch is attached.
>
> I'll send this out as a real v2 soon if no other issues pop up.
>
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
>
> https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
>
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally. However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
>
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
>
> To fix this, hide the entire nodeX/x86/ attribute group on
> systems without SGX support using the ->is_visible attribute
> group callback.
>
> Fixes: 50468e431335 ("x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node")
> Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> Cc: linux-sgx@xxxxxxxxxxxxxxx
> Cc: x86@xxxxxxxxxx
> Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> ---
>
> b/arch/x86/kernel/cpu/sgx/main.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr 2021-12-17 13:38:00.217312383 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c 2021-12-17 15:48:10.358932505 -0800
> @@ -910,6 +910,13 @@ static ssize_t sgx_total_bytes_show(stru
> }
> static DEVICE_ATTR_RO(sgx_total_bytes);
>
> +static umode_t arch_node_attr_is_visible(struct kobject * kobj,
> + struct attribute * attr, int idx)
> +{
> + /* Make all x86/ attributes invisible when SGX is not initialized: */
> + return !nodes_empty(sgx_numa_mask);
That's a very odd umode_t return value :)
Did you test this? What was the mode of the sysfs file that is created?
thanks,
greg k-h
