linux-kernel - Re: [PATCH 1/3] mm: introduce get_user_pages

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]

Date:   Fri, 10 Nov 2017 10:01:00 +0100
From:   Christoph Hellwig <hch@....de>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        Christoph Hellwig <hch@....de>, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] mm: introduce get_user_pages_longterm

> +long get_user_pages_longterm(unsigned long start, unsigned long nr_pages,
> +		unsigned int gup_flags, struct page **pages,
> +		struct vm_area_struct **vmas)
> +{
> +	struct vm_area_struct **__vmas = vmas;

How about calling the vma argument vma_arg, and the one used vma to
make thigns a little more readable?

> +	struct vm_area_struct *vma_prev = NULL;
> +	long rc, i;
> +
> +	if (!pages)
> +		return -EINVAL;
> +
> +	if (!vmas && IS_ENABLED(CONFIG_FS_DAX)) {
> +		__vmas = kzalloc(sizeof(struct vm_area_struct *) * nr_pages,
> +				GFP_KERNEL);
> +		if (!__vmas)
> +			return -ENOMEM;
> +	}
> +
> +	rc = get_user_pages(start, nr_pages, gup_flags, pages, __vmas);
> +
> +	/* skip scan for fs-dax vmas if they are compile time disabled */
> +	if (!IS_ENABLED(CONFIG_FS_DAX))
> +		goto out;

Instead of all this IS_ENABLED magic I'd recomment to just conditionally
compile this function and define it to get_user_pages in the header
if FS_DAX is disabled.

Else this looks fine to me.