[PATCH v2] staging: zsmalloc: Ensure handle is never 0 on success

[PATCH v2] staging: zsmalloc: Ensure handle is never 0 on success

[Olav Haugan]

zsmalloc encodes a handle using the pfn and an object
index. On hardware platforms with physical memory starting
at 0x0 the pfn can be 0. This causes the encoded handle to be
0 and is incorrectly interpreted as an allocation failure.

To prevent this false error we ensure that the encoded handle
will not be 0 when allocation succeeds.

Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
---
 drivers/staging/zsmalloc/zsmalloc-main.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 1a67537..3b950e5 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -430,7 +430,12 @@ static struct page *get_next_page(struct page *page)
 	return next;
 }
 
-/* Encode <page, obj_idx> as a single handle value */
+/*
+ * Encode <page, obj_idx> as a single handle value.
+ * On hardware platforms with physical memory starting at 0x0 the pfn
+ * could be 0 so we ensure that the handle will never be 0 by adjusting the
+ * encoded obj_idx value before encoding.
+ */
 static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
 {
 	unsigned long handle;
@@ -441,17 +446,21 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
 	}
 
 	handle = page_to_pfn(page) << OBJ_INDEX_BITS;
-	handle |= (obj_idx & OBJ_INDEX_MASK);
+	handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
 
 	return (void *)handle;
 }
 
-/* Decode <page, obj_idx> pair from the given object handle */
+/*
+ * Decode <page, obj_idx> pair from the given object handle. We adjust the
+ * decoded obj_idx back to its original value since it was adjusted in
+ * obj_location_to_handle().
+ */
 static void obj_handle_to_location(unsigned long handle, struct page **page,
 				unsigned long *obj_idx)
 {
 	*page = pfn_to_page(handle >> OBJ_INDEX_BITS);
-	*obj_idx = handle & OBJ_INDEX_MASK;
+	*obj_idx = (handle & OBJ_INDEX_MASK) - 1;
 }
 
 static unsigned long obj_idx_to_offset(struct page *page,
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] staging: zsmalloc: Ensure handle is never 0 on success

[Nitin Gupta]

On Thu, Nov 7, 2013 at 5:58 PM, Olav Haugan <ohaugan@codeaurora.org> wrote:
> zsmalloc encodes a handle using the pfn and an object
> index. On hardware platforms with physical memory starting
> at 0x0 the pfn can be 0. This causes the encoded handle to be
> 0 and is incorrectly interpreted as an allocation failure.
>
> To prevent this false error we ensure that the encoded handle
> will not be 0 when allocation succeeds.
>
> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
> ---
>  drivers/staging/zsmalloc/zsmalloc-main.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 1a67537..3b950e5 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -430,7 +430,12 @@ static struct page *get_next_page(struct page *page)
>         return next;
>  }
>
> -/* Encode <page, obj_idx> as a single handle value */
> +/*
> + * Encode <page, obj_idx> as a single handle value.
> + * On hardware platforms with physical memory starting at 0x0 the pfn
> + * could be 0 so we ensure that the handle will never be 0 by adjusting the
> + * encoded obj_idx value before encoding.
> + */
>  static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
>  {
>         unsigned long handle;
> @@ -441,17 +446,21 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
>         }
>
>         handle = page_to_pfn(page) << OBJ_INDEX_BITS;
> -       handle |= (obj_idx & OBJ_INDEX_MASK);
> +       handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>
>         return (void *)handle;
>  }
>
> -/* Decode <page, obj_idx> pair from the given object handle */
> +/*
> + * Decode <page, obj_idx> pair from the given object handle. We adjust the
> + * decoded obj_idx back to its original value since it was adjusted in
> + * obj_location_to_handle().
> + */
>  static void obj_handle_to_location(unsigned long handle, struct page **page,
>                                 unsigned long *obj_idx)
>  {
>         *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
> -       *obj_idx = handle & OBJ_INDEX_MASK;
> +       *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
>  }
>
>  static unsigned long obj_idx_to_offset(struct page *page,

Acked-by: Nitin Gupta <ngupta@vflare.org>

Thanks,
Nitin

Re: [PATCH v2] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Thu, Nov 07, 2013 at 05:58:03PM -0800, Olav Haugan wrote:
> zsmalloc encodes a handle using the pfn and an object
> index. On hardware platforms with physical memory starting
> at 0x0 the pfn can be 0. This causes the encoded handle to be
> 0 and is incorrectly interpreted as an allocation failure.

Please list the known hardware platforms that have this issue, so that
people have a chance to know if this patch is relevant for them or not.

For example, should I include this in the stable releases because it
affects systems that are shipping?  Or is it just in "future" chips and
it doesn't need to go there or not?

Please make it easy for me to do this type of determination, I already
asked you this question before, why didn't you include the information
here as well (hint, that is why I asked you...)

greg k-h

Re: [PATCH v2] staging: zsmalloc: Ensure handle is never 0 on success

[Olav Haugan]

Hi Greg,

On 11/11/2013 4:19 PM, Greg KH wrote:
> On Thu, Nov 07, 2013 at 05:58:03PM -0800, Olav Haugan wrote:
>> zsmalloc encodes a handle using the pfn and an object
>> index. On hardware platforms with physical memory starting
>> at 0x0 the pfn can be 0. This causes the encoded handle to be
>> 0 and is incorrectly interpreted as an allocation failure.
> 
> Please list the known hardware platforms that have this issue, so that
> people have a chance to know if this patch is relevant for them or not.
> 
> For example, should I include this in the stable releases because it
> affects systems that are shipping?  Or is it just in "future" chips and
> it doesn't need to go there or not?
> 
> Please make it easy for me to do this type of determination, I already
> asked you this question before, why didn't you include the information
> here as well (hint, that is why I asked you...)

I don't think it would be the best to mention specific hardware
platforms in the commit text. If I saw this patch listing specific
hardware platforms I would have made the wrong decision (I would look at
the list and decide that I am not running on those platforms so I don't
need this patch). The problem could happen on any hardware platform. It
just depends on how the memory map of the platform is configured. Hence,
I re-worded the commit text to make it clear that this will happen when
you have memory starting at 0x0.

If I list out specific hardware platforms it would be only a sample (I
do not know all hardware platforms and their memory maps). However,
having said that there are products already shipping with physical
address starting at 0.

Thanks,

Olav Haugan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Tue, Nov 12, 2013 at 09:06:37AM -0800, Olav Haugan wrote:
> Hi Greg,
> 
> On 11/11/2013 4:19 PM, Greg KH wrote:
> > On Thu, Nov 07, 2013 at 05:58:03PM -0800, Olav Haugan wrote:
> >> zsmalloc encodes a handle using the pfn and an object
> >> index. On hardware platforms with physical memory starting
> >> at 0x0 the pfn can be 0. This causes the encoded handle to be
> >> 0 and is incorrectly interpreted as an allocation failure.
> > 
> > Please list the known hardware platforms that have this issue, so that
> > people have a chance to know if this patch is relevant for them or not.
> > 
> > For example, should I include this in the stable releases because it
> > affects systems that are shipping?  Or is it just in "future" chips and
> > it doesn't need to go there or not?
> > 
> > Please make it easy for me to do this type of determination, I already
> > asked you this question before, why didn't you include the information
> > here as well (hint, that is why I asked you...)
> 
> I don't think it would be the best to mention specific hardware
> platforms in the commit text. If I saw this patch listing specific
> hardware platforms I would have made the wrong decision (I would look at
> the list and decide that I am not running on those platforms so I don't
> need this patch). The problem could happen on any hardware platform. It
> just depends on how the memory map of the platform is configured. Hence,
> I re-worded the commit text to make it clear that this will happen when
> you have memory starting at 0x0.
> 
> If I list out specific hardware platforms it would be only a sample (I
> do not know all hardware platforms and their memory maps). However,
> having said that there are products already shipping with physical
> address starting at 0.

Having something in there is better than nothing...

So, care to try it again?