Messages in this thread | | | Date | Thu, 11 Apr 2013 16:13:24 +0100 | From | Serban Constantinescu <> | Subject | Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer |
| |
On 10/04/13 23:12, Arve Hjønnevåg wrote: >>>> struct flat_binder_object { >>>> /* 8 bytes for large_flat_header. */ >>>> - unsigned long type; >>>> - unsigned long flags; >>>> + __u32 type; >>>> + __u32 flags; >>>> >>>> /* 8 bytes of data. */ >>>> union { >>>> void __user *binder; /* local object */ >>>> - signed long handle; /* remote object */ >>>> + __s32 handle; /* remote object */ >>> >>> >>> Why limit the handle to 32 bits when the pointer that it shares >>> storage with need to be 64 bit on 64 bit systems? >> >> >> Here I have mirrored the type being passed in handle - a file >> descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type == >> BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside >> the kernel/userspace(as 32bit value on 64bit systems). However this change >> does not limit the extension of the API since we can read the value as 64bit >> - binder(on 64bit systems). >> >> I can remove this change if you consider that is the better solution. >> > > I was asking if we should just use 64 bit handles on 64 bit systems, > not adding casts. It would require another union member for a file > descriptor however.
I will leave this handle as __s32 for this patch set but I will take a look into what and if we need to change this for a 64bit system. From a top level perspective(a look at binder_*_ref() functions and the userspace equivalent) this should work fine as 32bit.
>> >>>> }; >>>> >>>> /* extra data associated with local object */ >>>> @@ -67,18 +67,18 @@ struct flat_binder_object { >>>> */ >>>> >>>> struct binder_write_read { >>>> - signed long write_size; /* bytes to write */ >>>> - signed long write_consumed; /* bytes consumed by driver */ >>>> + size_t write_size; /* bytes to write */ >>>> + size_t write_consumed; /* bytes consumed by driver */ >>>> unsigned long write_buffer; >>>> - signed long read_size; /* bytes to read */ >>>> - signed long read_consumed; /* bytes consumed by driver */ >>>> + size_t read_size; /* bytes to read */ >>>> + size_t read_consumed; /* bytes consumed by driver */ >>> >>> >>> What is this change for? You changed from a signed type to an unsigned >>> type which seems unrelated to adding 64 bit support. >> >> >> See above explanation for binder_thread_write() change, I will break this >> into its own patch. >> >> >>>> unsigned long read_buffer; >>>> }; >>>> >>>> /* Use with BINDER_VERSION, driver fills in fields. */ >>>> struct binder_version { >>>> /* driver protocol version -- increment with incompatible change */ >>>> - signed long protocol_version; >>>> + __s32 protocol_version; >>> >>> >>> How does user-space know if it should use 32 bit or 64 bit pointers. >>> Without this change, the BINDER_VERSION ioctl would only match when >>> the size of long matches. >> >> >> The userspace can check the values returned by uname(). That will determine >> if the kernel is 32 or 64bit and depending on this select what binder >> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels >> using protocol_version as 64bit signed long(that is old kernel versions with >> no 64bit support). >> >> Leaving this value as signed long would mean that older versions of the >> binder(without 64bit support) will pass the check. Furthermore the protocol >> version will probably never exceed the values that could be represented on >> 32bit. It will also mean that BINDER_VERSION will have a different >> userspace/kernel handler for 64/32 systems. >> >> Let me know what are your thoughts related to these changes, >> Thanks for your feedback, >> Serban >> > > I think user-space should get the binder pointer size from the binder > driver, not elsewhere. Since the current driver does not appear to be > functional on a 64 bit system, I think adding an ioctl to get the > size, or encoding it into the binder version (use an unsigned type if > you do this), would be best.
I will think about the best way of getting the pointer size and add it to the patch set for binder compat. For this patch set I will only modify the protocol_version from long to __s32.
Thanks for your feedback, Serban
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |