Re: [PATCH v4] usb: gadget: f_fs: try to fix AIO issue under ARM 64 bit TAGGED mode

[Macpaul Lin <macpaul.lin@mediatek.com>]

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