linux-kernel - Re: [PATCH v2 3/7] mm, devm_memremap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f0cae82-130f-8a64-cfbd-fda5fd76bb79@deltatee.com>
Date:   Wed, 23 May 2018 09:47:45 -0600
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Dan Williams <dan.j.williams@...el.com>, akpm@...ux-foundation.org
Cc:     stable@...r.kernel.org, Christoph Hellwig <hch@....de>,
        Jérôme Glisse <jglisse@...hat.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/7] mm, devm_memremap_pages: Fix shutdown handling



On 22/05/18 11:10 PM, Dan Williams wrote:
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 7b4899c06f49..b5e894133cf6 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -106,6 +106,7 @@ typedef void (*dev_page_free_t)(struct page *page, void *data);
>   * @altmap: pre-allocated/reserved memory for vmemmap allocations
>   * @res: physical address range covered by @ref
>   * @ref: reference count that pins the devm_memremap_pages() mapping
> + * @kill: callback to transition @ref to the dead state
>   * @dev: host device of the mapping for debug
>   * @data: private data pointer for page_free()
>   * @type: memory type: see MEMORY_* in memory_hotplug.h
> @@ -117,13 +118,15 @@ struct dev_pagemap {
>  	bool altmap_valid;
>  	struct resource res;
>  	struct percpu_ref *ref;
> +	void (*kill)(struct percpu_ref *ref);
>  	struct device *dev;
>  	void *data;
>  	enum memory_type type;
>  };
>  
>  #ifdef CONFIG_ZONE_DEVICE
> -void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
> +void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap,
> +		void (*kill)(struct percpu_ref *));


It seems redundant to me to have the kill pointer both passed in as an
argument and passed in as part of pgmap... Why not just expect the user
to set it in the *pgmap that's passed in just like we expect ref to be
set ahead of time?

Another thought (that may be too forward looking) is to pass the
dev_pagemap struct to the kill function instead of the reference. That
way, if some future user wants to do something extra on kill they can
use container_of() to get extra context to work with.

Thanks,

Logan