[11/14] staging: android: ion: Allow heap name to be null

Submitted by Andrew F. Davis on Jan. 11, 2019, 6:05 p.m.

Details

Message ID 20190111180523.27862-12-afd@ti.com
State New
Series "Misc ION cleanups and adding unmapped heap"
Headers show

Commit Message

Andrew F. Davis Jan. 11, 2019, 6:05 p.m.
The heap name can be used for debugging but otherwise does not seem
to be required and no other part of the code will fail if left NULL
except here. We can make it required and check for it at some point,
for now lets just prevent this from causing a NULL pointer exception.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/staging/android/ion/ion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index bba5f682bc25..14e48f6eb734 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -467,7 +467,7 @@  static int ion_query_heaps(struct ion_heap_query *query)
 	max_cnt = query->cnt;
 
 	plist_for_each_entry(heap, &dev->heaps, node) {
-		strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
+		strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME);
 		hdata.name[sizeof(hdata.name) - 1] = '\0';
 		hdata.type = heap->type;
 		hdata.heap_id = heap->id;

Comments

Brian Starkey Jan. 16, 2019, 3:28 p.m.
Hi Andrew,

On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote:
> The heap name can be used for debugging but otherwise does not seem
> to be required and no other part of the code will fail if left NULL
> except here. We can make it required and check for it at some point,
> for now lets just prevent this from causing a NULL pointer exception.

I'm not so keen on this one. In the "new" API with heap querying, the
name string is the only way to identify the heap. I think Laura
mentioned at XDC2017 that it was expected that userspace should use
the strings to find the heap they want.

I'd actually be in favor of making the string a more strict UAPI than
allowing it to be empty (at least, if heap name strings is the API we
decide on for identifying heaps - which is another discussion).

Cheers,
-Brian

> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  drivers/staging/android/ion/ion.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index bba5f682bc25..14e48f6eb734 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query)
>  	max_cnt = query->cnt;
>  
>  	plist_for_each_entry(heap, &dev->heaps, node) {
> -		strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
> +		strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME);
>  		hdata.name[sizeof(hdata.name) - 1] = '\0';
>  		hdata.type = heap->type;
>  		hdata.heap_id = heap->id;
> -- 
> 2.19.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Andrew F. Davis Jan. 16, 2019, 5:12 p.m.
On 1/16/19 9:28 AM, Brian Starkey wrote:
> Hi Andrew,
> 
> On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote:
>> The heap name can be used for debugging but otherwise does not seem
>> to be required and no other part of the code will fail if left NULL
>> except here. We can make it required and check for it at some point,
>> for now lets just prevent this from causing a NULL pointer exception.
> 
> I'm not so keen on this one. In the "new" API with heap querying, the
> name string is the only way to identify the heap. I think Laura
> mentioned at XDC2017 that it was expected that userspace should use
> the strings to find the heap they want.
> 

Right now the names are only for debug. I accidentally left the name
null once and got a kernel crash. This is the only spot where it is
needed so I fixed it up. The other option is to make the name mandatory
and properly error out, I don't want to do that right now until the
below discussion is had to see if names really do matter or not.

> I'd actually be in favor of making the string a more strict UAPI than
> allowing it to be empty (at least, if heap name strings is the API we
> decide on for identifying heaps - which is another discussion).
> 

I would think identifying heaps by name would be less portable, but as
you said that is a whole different discussion..

Thanks,
Andrew

> Cheers,
> -Brian
> 
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>  drivers/staging/android/ion/ion.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index bba5f682bc25..14e48f6eb734 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query)
>>  	max_cnt = query->cnt;
>>  
>>  	plist_for_each_entry(heap, &dev->heaps, node) {
>> -		strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
>> +		strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME);
>>  		hdata.name[sizeof(hdata.name) - 1] = '\0';
>>  		hdata.type = heap->type;
>>  		hdata.heap_id = heap->id;
>> -- 
>> 2.19.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laura Abbott Jan. 18, 2019, 7:53 p.m.
On 1/16/19 9:12 AM, Andrew F. Davis wrote:
> On 1/16/19 9:28 AM, Brian Starkey wrote:
>> Hi Andrew,
>>
>> On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote:
>>> The heap name can be used for debugging but otherwise does not seem
>>> to be required and no other part of the code will fail if left NULL
>>> except here. We can make it required and check for it at some point,
>>> for now lets just prevent this from causing a NULL pointer exception.
>>
>> I'm not so keen on this one. In the "new" API with heap querying, the
>> name string is the only way to identify the heap. I think Laura
>> mentioned at XDC2017 that it was expected that userspace should use
>> the strings to find the heap they want.
>>
> 
> Right now the names are only for debug. I accidentally left the name
> null once and got a kernel crash. This is the only spot where it is
> needed so I fixed it up. The other option is to make the name mandatory
> and properly error out, I don't want to do that right now until the
> below discussion is had to see if names really do matter or not.
> 

Yes, the heap names are part of the query API and are the expected
way to identify individual heaps for the API at the moment so having
a null heap name is incorrect. The heap name seemed like the best way
to identify heaps to userspace but if you have an alternative proposal
I'd be interested.

Thanks,
Laura

>