Re: [PATCH v4 2/6] libnvdimm/namespace: Validate namespace start addr and size

[Dan Williams <dan.j.williams@intel.com>]

On Mon, Jan 20, 2020 at 6:08 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Make sure namespace start addr and size are properly aligned as per architecture
> restrictions. If the namespace is not aligned kernel mark the namespace 'disabled'
>
> Architectures like ppc64 use different page size than PAGE_SIZE to map
> direct-map address range. The kernel needs to make sure the namespace size is aligned
> correctly for the direct-map page size.
>
> kernel log will contain information as below.
>
> [    5.810939] nd_pmem namespace0.1: invalid size/SPA
> [    5.810969] nd_pmem: probe of namespace0.1 failed with error -95
>
> and the namespace will be marked 'disabled'
>
>   {
>     "dev":"namespace0.1",
>     "mode":"fsdax",
>     "map":"mem",
>     "size":1071644672,
>     "uuid":"25577a00-c012-421d-89ca-3ee189e08848",
>     "sector_size":512,
>     "state":"disabled"
>   },
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  drivers/nvdimm/namespace_devs.c | 50 +++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 032dc61725ff..0e2c90730ce3 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1113,6 +1113,51 @@ resource_size_t nvdimm_namespace_capacity(struct nd_namespace_common *ndns)
>  }
>  EXPORT_SYMBOL(nvdimm_namespace_capacity);
>
> +static bool nvdimm_valid_namespace(struct device *dev,
> +               struct nd_namespace_common *ndns, resource_size_t size)
> +{
> +       struct device *ndns_dev = &ndns->dev;
> +       struct nd_region *nd_region = to_nd_region(ndns->dev.parent);
> +       unsigned long map_size = arch_namespace_map_size();
> +       struct resource *res;
> +       u32 remainder;
> +
> +       /*
> +        * Don't validate the start and size for blk namespace type
> +        */

If you're going to comment I'd prefer a comment that explains why, not
what. E.g. "blk namespaces are ioremap()'d with small page aligned
apertures"

> +       if (is_namespace_blk(ndns_dev))
> +               return true;
> +
> +       /*
> +        * For btt and raw namespace kernel use ioremap. Assume both can work
> +        * with PAGE_SIZE alignment.
> +        */
> +       if (is_nd_btt(dev) || is_namespace_io(ndns_dev))
> +               map_size = PAGE_SIZE;
> +
> +       div_u64_rem(size, map_size * nd_region->ndr_mappings, &remainder);

Not all regions have mappings see e820_register_one(), so this is a
divide by zero.

I think this is mixing 2 different constraints. There's the soft
constraint of wanting aliased pmem capacity to remain aligned relative
to blk capacity, and then there's the hard constraint where the kernel
just can't use devm_memremap_pages() for the given capacity alignment.

I think this routine should be called:

    nvdimm_validate_geometry()

...and only check for:

    if (pmem_should_map_pages() || is_nd_pfn() || is_nd_dax())

...i.e namespaces are mapped by memremap_pages() and compare that
against memremap_compat_align().
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org