On Thu, 2020-02-27 at 09:55 +0000, Catalin Marinas wrote:
On Wed, Feb 26, 2020 at 08:01:52PM +0800, Macpaul Lin wrote:
This issue was found when adbd trying to open functionfs with AIO mode. Usually, we need to set "setprop sys.usb.ffs.aio_compat 0" to enable adbd with AIO mode on Android.
When adbd is opening functionfs, it will try to read 24 bytes at the first read I/O control. If this reading has been failed, adbd will try to send FUNCTIONFS_CLEAR_HALT to functionfs. When adbd is in AIO mode, functionfs will be acted with asyncronized I/O path. After the successful read transfer has been completed by gadget hardware, the following series of functions will be called. ffs_epfile_async_io_complete() -> ffs_user_copy_worker() -> copy_to_iter() -> _copy_to_iter() -> copyout() -> iterate_and_advance() -> iterate_iovec()
Adding debug trace to these functions, it has been found that in copyout(), access_ok() will check if the user space address is valid to write. However if CONFIG_ARM64_TAGGED_ADDR_ABI is enabled, adbd always passes user space address start with "0x3C" to gadget's AIO blocks. This tagged address will cause access_ok() check always fail. Which causes later calculation in iterate_iovec() turn zero. Copyout() won't copy data to user space since the length to be copied "v.iov_len" will be zero. Finally leads ffs_copy_to_iter() always return -EFAULT, causes adbd cannot open functionfs and send FUNCTIONFS_CLEAR_HALT.
Signed-off-by: Macpaul Lin macpaul.lin@mediatek.com Cc: Peter Chen peter.chen@nxp.com Cc: Catalin Marinas catalin.marinas@arm.com Cc: Miles Chen miles.chen@mediatek.com
Changes for v4:
- Abandon solution v3 by adding "TIF_TAGGED_ADDR" flag to gadget driver. According to Catalin's suggestion, change the solution by untagging user space address passed by AIO in gadget driver.
Well, this was suggested in case you have a strong reason not to do the untagging in adbd. As I said, tagged pointers in user space were supported long before we introduced CONFIG_ARM64_TAGGED_ADDR_ABI. How did adb cope with such tagged pointers before? It was not supposed to pass them to the kernel.
Thank for your explanation. Since adbd was developed by Google and we can only suggest (like, file an issue) to them. Here provides a temporary solution for other developer to solve there needs in a short period. Yes, I understood not supposed to pass those tagged pointers to kernel and will also explain this to Google adbd owners.
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index ce1d023..192935f 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -715,7 +715,20 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req) static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter) {
- ssize_t ret = copy_to_iter(data, data_len, iter);
- ssize_t ret;
+#if defined(CONFIG_ARM64)
- /*
* Replace tagged address passed by user space application before* copying.*/- if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
(iter->type == ITER_IOVEC)) {*(unsigned long *)&iter->iov->iov_base =(unsigned long)untagged_addr(iter->iov->iov_base);- }
+#endif
- ret = copy_to_iter(data, data_len, iter);
Here you should probably drop all the #ifdefs and IS_ENABLED checks since untagged_addr() is defined globally as a no-op (and overridden by arm64 and sparc).
Please don't send another patch until we understand (a) whether this is a user-space problem to fix or (b) if we fix it in the kernel, is this the only/right place? If we settle for the in-kernel untagging, do we explicitly untag the addresses in such kernel threads or we default to TIF_TAGGED_ADDR for all kernel threads, in case they ever call use_mm() (or we could even hook something in use_mm() to set this TIF flag temporarily).
Looking for feedback from the Android folk and a better analysis of the possible solution.
If we have any further update about this user space issue, I'll update the solution to this thread for other developers who need to solve this issue.
Thanks! Macpaul Lin