[PATCH 1/2] Staging: memrar: Moved memrar_allocator struct to memrar_allocator.c

[PATCH 1/2] Staging: memrar: Moved memrar_allocator struct to memrar_allocator.c

[Henri Häkkinen]

Forward declared memrar_allocator structure in memrar_allocator.h and
moved the definition to memrar_allocator.c. Implemented
memrar_allocator_capacity() and memrar_allocator_largest_free_area().

Signed-off-by: Henri Häkkinen <henuxd@gmail.com>
---
 drivers/staging/memrar/memrar_allocator.c |   43 ++++++++++++++++++++++++++
 drivers/staging/memrar/memrar_allocator.h |   48 ++++++++++++-----------------
 2 files changed, 63 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/memrar/memrar_allocator.c b/drivers/staging/memrar/memrar_allocator.c
index a4f8c58..cb74e3c 100644
--- a/drivers/staging/memrar/memrar_allocator.c
+++ b/drivers/staging/memrar/memrar_allocator.c
@@ -41,6 +41,36 @@
 #include <linux/kernel.h>
 
 
+/**
+ * struct memrar_allocator - encapsulation of the memory allocator state
+ * @lock:		Lock used to synchronize access to the memory
+ *			allocator state.
+ * @base:		Base (start) address of the allocator memory
+ *			space.
+ * @capacity:		Size of the allocator memory space in bytes.
+ * @block_size:		The size in bytes of individual blocks within
+ *			the allocator memory space.
+ * @largest_free_area:	Largest free area of memory in the allocator
+ *			in bytes.
+ * @allocated_list:	List of allocated memory block address
+ *			ranges.
+ * @free_list:		List of free address ranges.
+ *
+ * This structure contains all memory allocator state, including the
+ * base address, capacity, free list, lock, etc.
+ */
+struct memrar_allocator {
+/* private: internal use only */
+	struct mutex lock;
+	unsigned long base;
+	size_t capacity;
+	size_t block_size;
+	size_t largest_free_area;
+	struct memrar_address_ranges allocated_list;
+	struct memrar_address_ranges free_list;
+};
+
+
 struct memrar_allocator *memrar_create_allocator(unsigned long base,
 						 size_t capacity,
 						 size_t block_size)
@@ -423,6 +453,19 @@ exit_memrar_free:
 	return 0;
 }
 
+size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
+{
+	if (allocator == NULL)
+		return 0;
+	return allocator->largest_free_area;
+}
+
+size_t memrar_allocator_capacity(struct memrar_allocator *allocator)
+{
+	if (allocator == NULL)
+		return 0;
+	return allocator->capacity;
+}
 
 
 /*
diff --git a/drivers/staging/memrar/memrar_allocator.h b/drivers/staging/memrar/memrar_allocator.h
index 0b80dea..49dbc6e 100644
--- a/drivers/staging/memrar/memrar_allocator.h
+++ b/drivers/staging/memrar/memrar_allocator.h
@@ -50,34 +50,10 @@ struct memrar_address_ranges {
 	struct memrar_address_range range;
 };
 
-/**
- * struct memrar_allocator - encapsulation of the memory allocator state
- * @lock:		Lock used to synchronize access to the memory
- *			allocator state.
- * @base:		Base (start) address of the allocator memory
- *			space.
- * @capacity:		Size of the allocator memory space in bytes.
- * @block_size:		The size in bytes of individual blocks within
- *			the allocator memory space.
- * @largest_free_area:	Largest free area of memory in the allocator
- *			in bytes.
- * @allocated_list:	List of allocated memory block address
- *			ranges.
- * @free_list:		List of free address ranges.
- *
- * This structure contains all memory allocator state, including the
- * base address, capacity, free list, lock, etc.
- */
-struct memrar_allocator {
-/* private: internal use only */
-	struct mutex lock;
-	unsigned long base;
-	size_t capacity;
-	size_t block_size;
-	size_t largest_free_area;
-	struct memrar_address_ranges allocated_list;
-	struct memrar_address_ranges free_list;
-};
+
+/* Forward declaration */
+struct memrar_allocator;
+
 
 /**
  * memrar_create_allocator() - create a memory allocator
@@ -139,6 +115,22 @@ unsigned long memrar_allocator_alloc(struct memrar_allocator *allocator,
 long memrar_allocator_free(struct memrar_allocator *allocator,
 			   unsigned long address);
 
+/**
+ * memrar_allocator_largest_area() - largest free area of memory
+ * @allocator:  The allocator instance the free area is returned for.
+ *
+ * Return the largest free area of memory in the allocator in bytes.
+ */
+size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator);
+
+/**
+ * memrar_allocator_capacity() - size of the allocator memory
+ * @allocator:  The allocator instance the capicity is returned for.
+ *
+ * Return the size of the allocator memory space in bytes.
+ */
+size_t memrar_allocator_capacity(struct memrar_allocator *allocator);
+
 #endif  /* MEMRAR_ALLOCATOR_H */
 
 
-- 
1.7.1

[PATCH 2/2] Staging: memrar: Fixed memrar_handler.c

[Henri Häkkinen]

Fixed memrar_handler.c to use the new memrar_allocator API.  Removed
the corresponding issue from TODO.  Implemented locking in
memrar_allocator_largest_free_area().

Signed-off-by: Henri Häkkinen <henuxd@gmail.com
---
 drivers/staging/memrar/TODO               |   16 +---------------
 drivers/staging/memrar/memrar_allocator.c |   12 +++++++++---
 drivers/staging/memrar/memrar_handler.c   |   13 ++++---------
 3 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/memrar/TODO b/drivers/staging/memrar/TODO
index 0087447..9659aab 100644
--- a/drivers/staging/memrar/TODO
+++ b/drivers/staging/memrar/TODO
@@ -16,21 +16,7 @@ memrar_allocator.[ch]
 ---------------------
 1. Address potential fragmentation issues with the memrar_allocator.
 
-2. Hide struct memrar_allocator details/fields.  They need not be
-   exposed to the user.
-     a. Forward declare struct memrar_allocator.
-     b. Move all three struct definitions to `memrar_allocator.c'
-        source file.
-     c. Add a memrar_allocator_largest_free_area() function, or
-        something like that to get access to the value of the struct
-        memrar_allocator "largest_free_area" field.  This allows the
-        struct memrar_allocator fields to be completely hidden from
-        the user.  The memrar_handler code really only needs this for
-        statistic gathering on-demand.
-     d. Do the same for the "capacity" field as the
-        "largest_free_area" field.
-
-3. Move memrar_allocator.* to kernel `lib' directory since it is HW
+2. Move memrar_allocator.* to kernel `lib' directory since it is HW
    neutral.
      a. Alternatively, use lib/genalloc.c instead.
      b. A kernel port of Doug Lea's malloc() implementation may also
diff --git a/drivers/staging/memrar/memrar_allocator.c b/drivers/staging/memrar/memrar_allocator.c
index cb74e3c..924eab3 100644
--- a/drivers/staging/memrar/memrar_allocator.c
+++ b/drivers/staging/memrar/memrar_allocator.c
@@ -455,9 +455,15 @@ exit_memrar_free:
 
 size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
 {
-	if (allocator == NULL)
-		return 0;
-	return allocator->largest_free_area;
+	size_t tmp = 0;
+
+	if (allocator != NULL) {
+		mutex_lock(&allocator->lock);
+		tmp = allocator->largest_free_area;
+		mutex_unlock(&allocator->lock);
+	}
+
+	return tmp;
 }
 
 size_t memrar_allocator_capacity(struct memrar_allocator *allocator)
diff --git a/drivers/staging/memrar/memrar_handler.c b/drivers/staging/memrar/memrar_handler.c
index 22208cd..a652593 100644
--- a/drivers/staging/memrar/memrar_handler.c
+++ b/drivers/staging/memrar/memrar_handler.c
@@ -351,7 +351,8 @@ static int memrar_init_rar_resources(int rarnum, char const *devname)
 		devname, rarnum, (unsigned long) low, (unsigned long) high);
 
 	pr_info("%s: BRAR[%d] size = %zu KiB\n",
-			devname, rarnum, rar->allocator->capacity / 1024);
+			devname, rarnum,
+			memrar_allocator_capacity(rar->allocator) / 1024);
 
 	rar->allocated = 1;
 	return 0;
@@ -542,15 +543,9 @@ static long memrar_get_stat(struct RAR_stat *r)
 
 	BUG_ON(allocator == NULL);
 
-	/*
-	 * Allocator capacity doesn't change over time.  No
-	 * need to synchronize.
-	 */
-	r->capacity = allocator->capacity;
+	r->capacity = memrar_allocator_capacity(allocator);
+	r->largest_block_size = memrar_allocator_largest_free_area(allocator);
 
-	mutex_lock(&allocator->lock);
-	r->largest_block_size = allocator->largest_free_area;
-	mutex_unlock(&allocator->lock);
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCH 2/2] Staging: memrar: Fixed memrar_handler.c

[Alan Cox]

>  size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
>  {
> -	if (allocator == NULL)
> -		return 0;
> -	return allocator->largest_free_area;
> +	size_t tmp = 0;
> +
> +	if (allocator != NULL) {
> +		mutex_lock(&allocator->lock);
> +		tmp = allocator->largest_free_area;
> +		mutex_unlock(&allocator->lock);

This doesn't seem to make any sense (in either version). The moment you
drop the lock the value in "tmp" becomes stale as the allocator could
change it. ?

Re: [PATCH 1/2] Staging: memrar: Moved memrar_allocator struct to memrar_allocator.c

[Alan Cox]

> +size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
> +{
> +	if (allocator == NULL)
> +		return 0;
> +	return allocator->largest_free_area;
> +}

static ?

Re: [PATCH 1/2] Staging: memrar: Moved memrar_allocator struct to memrar_allocator.c

[Henri Häkkinen]

On 24.6.2010, at 12.16, Alan Cox wrote:

>> +size_t memrar_allocator_largest_free_area(struct memrar_allocator *allocator)
>> +{
>> +	if (allocator == NULL)
>> +		return 0;
>> +	return allocator->largest_free_area;
>> +}
> 
> static ?

The entire point of these two patches was to hide the memrar_allocator structure by moving it to memrar_allocator.c, forward declaring it in memrar_allocator.h and then providing accessor functions memrar_allocator_largest_free_area and memrar_allocator_capacity (this was the #2 issue for memrar_allocator.[ch] in the TODO file). By defining memrar_allocator_largest_free_area function as static (this is what you mean, right?) would make no sense.

RE: [PATCH 2/2] Staging: memrar: Fixed memrar_handler.c

[Othman, Ossama]

Hi Alan,

> >  size_t memrar_allocator_largest_free_area(struct memrar_allocator
> *allocator)
> >  {
> > -	if (allocator == NULL)
> > -		return 0;
> > -	return allocator->largest_free_area;
> > +	size_t tmp = 0;
> > +
> > +	if (allocator != NULL) {
> > +		mutex_lock(&allocator->lock);
> > +		tmp = allocator->largest_free_area;
> > +		mutex_unlock(&allocator->lock);
> 
> This doesn't seem to make any sense (in either version). The moment you
> drop the lock the value in "tmp" becomes stale as the allocator could
> change it. ?

Agreed, but I don't think there's anything we can do about it with the current interface.  We will always have a time-of-check-time-of-use TOCTOU race with this function as it stands.  I pointed out the issue in my response to Henri as well, but I focused more on hiding the lock from the caller.  The TOCTOU race exists in both versions, i.e. the old code and proposed new code.  The value returned from this function in this particular case will ultimately be passed to the user when the user issues a RAR_HANDLER_STAT ioctl to the /dev/memrar device.  For the most part, this function is only used for debugging.  It is not needed for general operation of the memrar_allocator or the memrar_handler (/dev/memrar).

-Ossama