linux-kernel - Re: [PATCH 1/2] mm: cma: split out in

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: <20160219132323.d3c6bfb8cf1a420b4cb1b508@linux-foundation.org>
Date:	Fri, 19 Feb 2016 13:23:23 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Rabin Vincent <rabin.vincent@...s.com>
Cc:	linux@....linux.org.uk, mina86@...a86.com,
	linux-arm-kernel@...ts.infradead.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Rabin Vincent <rabinv@...s.com>
Subject: Re: [PATCH 1/2] mm: cma: split out in_cma check to separate
 function

On Fri, 19 Feb 2016 09:12:03 +0100 Rabin Vincent <rabin.vincent@...s.com> wrote:

> Split out the logic in cma_release() which checks if the page is in the
> contiguous area to a new function which can be called separately.  ARM
> will use this.
> 
> ...
>
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -27,5 +27,17 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
>  					unsigned int order_per_bit,
>  					struct cma **res_cma);
>  extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align);
> +
>  extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
> +#ifdef CONFIG_CMA
> +extern bool in_cma(struct cma *cma, const struct page *pages,
> +		   unsigned int count);
> +#else
> +static inline bool in_cma(struct cma *cma, const struct page *pages,
> +			  unsigned int count)
> +{
> +	return false;
> +}
> +#endif

Calling it "pages" is weird.  I immediately read it as a `struct page **'. 
Drop the 's' please.  Or call it `start_page' if you wish to retain the
"we're dealing with more than one page here" info.

And `nr_pages' is a better name than `count'.  

And `in_cma' seems rather ...  brief.  And it breaks the convention that
interface identifiers start with the name of the subsystem.  Look at the rest
of cma.h: cma_get_base(), cma_get_size() cma_declare_contiguous(), etc -
let's not break that.

>  #endif
> diff --git a/mm/cma.c b/mm/cma.c
> index ea506eb..55cda16 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -426,6 +426,23 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align)
>  	return page;
>  }
>  
> +bool in_cma(struct cma *cma, const struct page *pages, unsigned int count)

A bit of documentation would be nice.

> +{
> +	unsigned long pfn;
> +
> +	if (!cma || !pages)
> +		return false;

Is this actually needed?  If there's no good reason for the test, let's leave
it out because it will just be hiding bugs in the caller.

> +	pfn = page_to_pfn(pages);
> +
> +	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
> +		return false;
> +
> +	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
> +
> +	return true;
> +}
> +
>  /**
>   * cma_release() - release allocated pages
>   * @cma:   Contiguous memory region for which the allocation is performed.

Apart from those cosmeticish things, no objections from me.