[PATCH] cma: use unsigned type for count argument

[PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

Specifying negative size of buffer makes no sense and thus this commit
changes the type of the count argument to unsigned.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 arch/arm/mm/dma-mapping.c      |   12 ++++++------
 drivers/base/dma-contiguous.c  |    6 +++---
 include/linux/dma-contiguous.h |    8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 6b2fb87..77e292e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 					  gfp_t gfp, struct dma_attrs *attrs)
 {
 	struct page **pages;
-	int count = size >> PAGE_SHIFT;
-	int array_size = count * sizeof(struct page *);
-	int i = 0;
+	unsigned int count = size >> PAGE_SHIFT;
+	unsigned int array_size = count * sizeof(struct page *);
+	unsigned int i = 0;
 
 	if (array_size <= PAGE_SIZE)
 		pages = kzalloc(array_size, gfp);
@@ -1102,9 +1102,9 @@ error:
 static int __iommu_free_buffer(struct device *dev, struct page **pages,
 			       size_t size, struct dma_attrs *attrs)
 {
-	int count = size >> PAGE_SHIFT;
-	int array_size = count * sizeof(struct page *);
-	int i;
+	unsigned int count = size >> PAGE_SHIFT;
+	unsigned int array_size = count * sizeof(struct page *);
+	unsigned int i;
 
 	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
 		dma_release_from_contiguous(dev, pages[0], count);
diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index 0ca5442..e34e3e0 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -303,7 +303,7 @@ err:
  * global one. Requires architecture specific get_dev_cma_area() helper
  * function.
  */
-struct page *dma_alloc_from_contiguous(struct device *dev, int count,
+struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
 				       unsigned int align)
 {
 	unsigned long mask, pfn, pageno, start = 0;
@@ -317,7 +317,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, int count,
 	if (align > CONFIG_CMA_ALIGNMENT)
 		align = CONFIG_CMA_ALIGNMENT;
 
-	pr_debug("%s(cma %p, count %d, align %d)\n", __func__, (void *)cma,
+	pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
 		 count, align);
 
 	if (!count)
@@ -364,7 +364,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, int count,
  * true otherwise.
  */
 bool dma_release_from_contiguous(struct device *dev, struct page *pages,
-				 int count)
+				 unsigned int count)
 {
 	struct cma *cma = dev_get_cma_area(dev);
 	unsigned long pfn;
diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
index 01b5c84..040501a 100644
--- a/include/linux/dma-contiguous.h
+++ b/include/linux/dma-contiguous.h
@@ -71,10 +71,10 @@ void dma_contiguous_reserve(phys_addr_t addr_limit);
 int dma_declare_contiguous(struct device *dev, phys_addr_t size,
 			   phys_addr_t base, phys_addr_t limit);
 
-struct page *dma_alloc_from_contiguous(struct device *dev, int count,
+struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
 				       unsigned int order);
 bool dma_release_from_contiguous(struct device *dev, struct page *pages,
-				 int count);
+				 unsigned int count);
 
 #else
 
@@ -90,7 +90,7 @@ int dma_declare_contiguous(struct device *dev, phys_addr_t size,
 }
 
 static inline
-struct page *dma_alloc_from_contiguous(struct device *dev, int count,
+struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
 				       unsigned int order)
 {
 	return NULL;
@@ -98,7 +98,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, int count,
 
 static inline
 bool dma_release_from_contiguous(struct device *dev, struct page *pages,
-				 int count)
+				 unsigned int count)
 {
 	return false;
 }
-- 
1.7.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] cma: use unsigned type for count argument

[Andrew Morton]

On Wed, 19 Dec 2012 18:44:40 +0100
Michal Nazarewicz <mpn@google.com> wrote:

> From: Michal Nazarewicz <mina86@mina86.com>
> 
> Specifying negative size of buffer makes no sense and thus this commit
> changes the type of the count argument to unsigned.
> 
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
>  					  gfp_t gfp, struct dma_attrs *attrs)
>  {
>  	struct page **pages;
> -	int count = size >> PAGE_SHIFT;
> -	int array_size = count * sizeof(struct page *);
> -	int i = 0;
> +	unsigned int count = size >> PAGE_SHIFT;
> +	unsigned int array_size = count * sizeof(struct page *);
> +	unsigned int i = 0;

C programmers expect a variable called `i' to have type `int'.  It
would be clearer to find a new name for this.  `idx', perhaps.

Also, we later do

	int j;
	..
	pages[i + j] ...

So `j' should also be converted to unsigned.  And perhaps renamed, but
`j' isn't as egregious as `i'.

>  	if (array_size <= PAGE_SIZE)
>  		pages = kzalloc(array_size, gfp);
> @@ -1102,9 +1102,9 @@ error:
>  static int __iommu_free_buffer(struct device *dev, struct page **pages,
>  			       size_t size, struct dma_attrs *attrs)
>  {
> -	int count = size >> PAGE_SHIFT;
> -	int array_size = count * sizeof(struct page *);
> -	int i;
> +	unsigned int count = size >> PAGE_SHIFT;
> +	unsigned int array_size = count * sizeof(struct page *);
> +	unsigned int i;

ditto.


--- a/arch/arm/mm/dma-mapping.c~cma-use-unsigned-type-for-count-argument-fix
+++ a/arch/arm/mm/dma-mapping.c
@@ -1040,7 +1040,7 @@ static struct page **__iommu_alloc_buffe
 	struct page **pages;
 	unsigned int count = size >> PAGE_SHIFT;
 	unsigned int array_size = count * sizeof(struct page *);
-	unsigned int i = 0;
+	unsigned int idx = 0;
 
 	if (array_size <= PAGE_SIZE)
 		pages = kzalloc(array_size, gfp);
@@ -1049,8 +1049,7 @@ static struct page **__iommu_alloc_buffe
 	if (!pages)
 		return NULL;
 
-	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs))
-	{
+	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
 		unsigned long order = get_order(size);
 		struct page *page;
 
@@ -1060,38 +1059,39 @@ static struct page **__iommu_alloc_buffe
 
 		__dma_clear_buffer(page, size);
 
-		for (i = 0; i < count; i++)
-			pages[i] = page + i;
+		for (idx = 0; idx < count; idx++)
+			pages[i] = page + idx;
 
 		return pages;
 	}
 
 	while (count) {
-		int j, order = __fls(count);
+		unsigned int j;
+		unnsigned int order = __fls(count);
 
-		pages[i] = alloc_pages(gfp | __GFP_NOWARN, order);
-		while (!pages[i] && order)
-			pages[i] = alloc_pages(gfp | __GFP_NOWARN, --order);
-		if (!pages[i])
+		pages[idx] = alloc_pages(gfp | __GFP_NOWARN, order);
+		while (!pages[idx] && order)
+			pages[idx] = alloc_pages(gfp | __GFP_NOWARN, --order);
+		if (!pages[idx])
 			goto error;
 
 		if (order) {
-			split_page(pages[i], order);
+			split_page(pages[idx], order);
 			j = 1 << order;
 			while (--j)
-				pages[i + j] = pages[i] + j;
+				pages[idx + j] = pages[idx] + j;
 		}
 
-		__dma_clear_buffer(pages[i], PAGE_SIZE << order);
-		i += 1 << order;
+		__dma_clear_buffer(pages[idx], PAGE_SIZE << order);
+		idx += 1 << order;
 		count -= 1 << order;
 	}
 
 	return pages;
 error:
-	while (i--)
-		if (pages[i])
-			__free_pages(pages[i], 0);
+	while (idx--)
+		if (pages[idx])
+			__free_pages(pages[idx], 0);
 	if (array_size <= PAGE_SIZE)
 		kfree(pages);
 	else
@@ -1104,14 +1104,15 @@ static int __iommu_free_buffer(struct de
 {
 	unsigned int count = size >> PAGE_SHIFT;
 	unsigned int array_size = count * sizeof(struct page *);
-	unsigned int i;
 
 	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
 		dma_release_from_contiguous(dev, pages[0], count);
 	} else {
-		for (i = 0; i < count; i++)
-			if (pages[i])
-				__free_pages(pages[i], 0);
+		unsigned int idx;
+
+		for (idx = 0; idx < count; idx++)
+			if (pages[idx])
+				__free_pages(pages[idx], 0);
 	}
 
 	if (array_size <= PAGE_SIZE)
diff -puN drivers/base/dma-contiguous.c~cma-use-unsigned-type-for-count-argument-fix drivers/base/dma-contiguous.c
diff -puN include/linux/dma-contiguous.h~cma-use-unsigned-type-for-count-argument-fix include/linux/dma-contiguous.h
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] cma: use unsigned type for count argument

[David Rientjes]

On Thu, 20 Dec 2012, Andrew Morton wrote:

> > Specifying negative size of buffer makes no sense and thus this commit
> > changes the type of the count argument to unsigned.
> > 
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
> >  					  gfp_t gfp, struct dma_attrs *attrs)
> >  {
> >  	struct page **pages;
> > -	int count = size >> PAGE_SHIFT;
> > -	int array_size = count * sizeof(struct page *);
> > -	int i = 0;
> > +	unsigned int count = size >> PAGE_SHIFT;
> > +	unsigned int array_size = count * sizeof(struct page *);
> > +	unsigned int i = 0;
> 
> C programmers expect a variable called `i' to have type `int'.  It
> would be clearer to find a new name for this.  `idx', perhaps.
> 

I didn't ack this because there's no bounds checking on 
dma_alloc_from_contiguous() and bitmap_set() has a dangerous side-effect 
when called with an overflowed nr since it takes a signed argument.  
Marek, is there some sane upper bound we can put on count?

Additionally, I think at least this is needed for callers of bitmap_set() 
for some sanity (unless someone wants to audit the almost 100 callers and 
change it to unsigned as well).  There's probably additional nastiness in 
this library as well, I didn't check.
---
diff --git a/lib/bitmap.c b/lib/bitmap.c
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -287,7 +287,7 @@ void bitmap_set(unsigned long *map, int start, int nr)
 		mask_to_set = ~0UL;
 		p++;
 	}
-	if (nr) {
+	if (nr > 0) {
 		mask_to_set &= BITMAP_LAST_WORD_MASK(size);
 		*p |= mask_to_set;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 4221 bytes --]

On Fri, Dec 21 2012, David Rientjes wrote:
> > Specifying negative size of buffer makes no sense and thus this commit
> > changes the type of the count argument to unsigned.
> > 
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
> >  					  gfp_t gfp, struct dma_attrs *attrs)
> >  {
> >  	struct page **pages;
> > -	int count = size >> PAGE_SHIFT;
> > -	int array_size = count * sizeof(struct page *);
> > -	int i = 0;
> > +	unsigned int count = size >> PAGE_SHIFT;
> > +	unsigned int array_size = count * sizeof(struct page *);
> > +	unsigned int i = 0;

> I didn't ack this because there's no bounds checking on 
> dma_alloc_from_contiguous() and bitmap_set() has a dangerous side-effect 
> when called with an overflowed nr since it takes a signed argument.  

Mystery solved.  I recalled that there was some reason why the count is
specified as a signed int and thought bitmap_find_next_zero_area() was
the culprit, but now it seems that bitmap_set() was the reason.

> Marek, is there some sane upper bound we can put on count?

INT_MAX would be sufficient.  After all, it maps to a 8 TiB buffer (if
page is 4 KiB).

Moreover, in reality, the few places that call
dma_alloc_from_contiguous() pass a value that cannot be higher than
INT_MAX, ie. (listings heavily stripped):

arch/arm/mm/dma-mapping.c-static void *__alloc_from_contiguous(struct device *dev, size_t size,
arch/arm/mm/dma-mapping.c-                                   pgprot_t prot, struct page **ret_page)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-      size_t count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c:      page = dma_alloc_from_contiguous(dev, count, order);
arch/arm/mm/dma-mapping.c-}

arch/arm/mm/dma-mapping.c-static void *__alloc_from_contiguous(struct device *dev, size_t size,
arch/arm/mm/dma-mapping.c-                                   pgprot_t prot, struct page **ret_page)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-      size_t count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c:      page = dma_alloc_from_contiguous(dev, count, order);
arch/arm/mm/dma-mapping.c-}

arch/arm/mm/dma-mapping.c-static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
arch/arm/mm/dma-mapping.c-                                        gfp_t gfp, struct dma_attrs *attrs)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-      unsigned int count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c-      if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
arch/arm/mm/dma-mapping.c:              page = dma_alloc_from_contiguous(dev, count, order);
arch/arm/mm/dma-mapping.c-      }
arch/arm/mm/dma-mapping.c-}

arch/x86/kernel/pci-dma.c-void *dma_generic_alloc_coherent(struct device *dev, size_t size,
arch/x86/kernel/pci-dma.c-                               dma_addr_t *dma_addr, gfp_t flag,
arch/x86/kernel/pci-dma.c-                               struct dma_attrs *attrs)
arch/x86/kernel/pci-dma.c-{
arch/x86/kernel/pci-dma.c-      unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
arch/x86/kernel/pci-dma.c-      if (!(flag & GFP_ATOMIC))
arch/x86/kernel/pci-dma.c:              page = dma_alloc_from_contiguous(dev, count, get_order(size));
arch/x86/kernel/pci-dma.c-}

So I think just adding the following, should be sufficient to make
everyone happy:

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index e34e3e0..e91743b 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -320,7 +320,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
 	pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
 		 count, align);
 
-	if (!count)
+	if (!count || count > INT_MAX)
 		return NULL;
 
 	mask = (1 << align) - 1;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] cma: use unsigned type for count argument

[David Rientjes]

On Sat, 22 Dec 2012, Michal Nazarewicz wrote:

> So I think just adding the following, should be sufficient to make
> everyone happy:
> 
> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
> index e34e3e0..e91743b 100644
> --- a/drivers/base/dma-contiguous.c
> +++ b/drivers/base/dma-contiguous.c
> @@ -320,7 +320,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
>  	pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
>  		 count, align);
>  
> -	if (!count)
> +	if (!count || count > INT_MAX)
>  		return NULL;
>  
>  	mask = (1 << align) - 1;

How is this different than leaving the formal to have a signed type, i.e. 
drop your patch, and testing for count <= 0 instead?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

> On Sat, 22 Dec 2012, Michal Nazarewicz wrote:
>> So I think just adding the following, should be sufficient to make
>> everyone happy:
>> 
>> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
>> index e34e3e0..e91743b 100644
>> --- a/drivers/base/dma-contiguous.c
>> +++ b/drivers/base/dma-contiguous.c
>> @@ -320,7 +320,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
>>  	pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
>>  		 count, align);
>>  
>> -	if (!count)
>> +	if (!count || count > INT_MAX)
>>  		return NULL;
>>  
>>  	mask = (1 << align) - 1;
>
On Thu, Dec 27 2012, David Rientjes <rientjes@google.com> wrote:
> How is this different than leaving the formal to have a signed type, i.e. 
> drop your patch, and testing for count <= 0 instead?

Not much different I guess.  I don't have strong opinions to be honest,
except that I feel unsigned is the proper type to use, on top of which
I think bitmap_set() should use unsigned, so in case anyone ever bothers
to change it, CMA will be ready. :P

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]