On Tue, 2020-03-03 at 11:19 -0800, Evgenii Stepanov wrote:

I'm a bit surprised that this is necessary, given that the earlier patch that added the (current->flags & PF_KTHREAD) condition was in response to this exact problem, and I know for sure that it helped. This was the stack trace for the call to __range_ok in that case: [ 12.886765] c1 271 _copy_to_iter+0xb8/0x5c0

[ 12.891421] c1 271 ffs_user_copy_worker+0xec/0x24c [ 12.896699] c1 271 process_one_work+0x264/0x450 [ 12.901713] c1 271 worker_thread+0x250/0x484 [ 12.906454] c1 271 kthread+0x11c/0x12c [ 12.910664] c1 271 ret_from_fork+0x10/0x18

It would be great to know what changed to require the updated condition.

Adding a prctl call to adb is unlikely to help, because it would not stop tagged address generation in malloc.

Sorry for late reply, after carefully check the kerenl update status in Mediatek's branch. I've found we got this patch (df325e05a682 ("arm64: Validate tagged addresses in access_ok() called from kernel threads")) updated into internal Mediatek's working tree around Feb 23 or 24. However, I'm not sure why that patch cannot work in my own working tree at that time. I've indeed dumped user space address and checked the return result in access_ok() and found it was not worked.

In these days I've clean up all my working space and re-test this patch, I've found to check PF_KTHREAD and TIF_TAGGED_ADDR was enough to solve this problem. Sorry for bothering I'm not sure what causes that fail in previous environment.

Moreover, I've tested PF_WQ_WORKER case, if we replaced test flag PF_KTHREAD by PF_WQ_WORKER, AIO will still be worked, too. Both code

A. if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) && (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))

or

B. if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) && (current->flags & PF_WQ_WORKER || test_thread_flag(TIF_TAGGED_ADDR)))

are worked for this issue.

On Mon, Mar 2, 2020 at 8:19 AM Catalin Marinas catalin.marinas@arm.com wrote:

    On Sun, Mar 01, 2020 at 11:20:43AM +0800, Macpaul Lin wrote:
    > On Fri, 2020-02-28 at 16:48 +0000, Catalin Marinas wrote:
    > > On Wed, Feb 26, 2020 at 08:01:52PM +0800, Macpaul Lin
    wrote:
    > > > 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);
    > > >   if (likely(ret == data_len))
    > > >           return ret;
    > > 
    > > I had forgotten that we discussed a similar case already a
    few months
    > > ago (thanks to Evgenii for pointing out). Do you have this
    commit
    > > applied to your tree: df325e05a682 ("arm64: Validate
    tagged addresses in
    > > access_ok() called from kernel threads")?
    > > 
    > 
    > Yes! We have that patch. I've also got Google's reply about
    referencing
    > this patch in android kernel tree.
    >
    https://android-review.googlesource.com/c/kernel/common/+/1186615
    > 
    > However, during my debugging process, I've dumped specific
    length (e.g.,
    > 24 bytes for the first request) AIO request buffer address
    both in adbd
    > and in __range_ok(). Then I've found __range_ok() still
    always return
    > false on address begin with "0x3c". Since untagged_addr()
    already called
    > in __range_ok(), to set "TIF_TAGGED_ADDR" with adbd's user
    space buffer
    > should be the possible solution. Hence I've send the v3
    patch.
    
    ffs_copy_to_iter() is called from a workqueue
    (ffs_user_copy_worker()).
    That's still in a kernel thread context but it doesn't have
    PF_KTHREAD
    set, hence __range_ok() rejects the tagged address. Can you
    try the diff
    below:
    
    diff --git a/arch/arm64/include/asm/uaccess.h
    b/arch/arm64/include/asm/uaccess.h
    index 32fc8061aa76..2803143cad1f 100644
    --- a/arch/arm64/include/asm/uaccess.h
    +++ b/arch/arm64/include/asm/uaccess.h
    @@ -68,7 +68,8 @@ static inline unsigned long __range_ok(const
    void __user *addr, unsigned long si
             * the user address before checking.
             */
            if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
    -           (current->flags & PF_KTHREAD ||
    test_thread_flag(TIF_TAGGED_ADDR)))
    +           (current->flags & (PF_KTHREAD | PF_WQ_WORKER) ||
    +            test_thread_flag(TIF_TAGGED_ADDR)))
                    addr = untagged_addr(addr);
    
            __chk_user_ptr(addr);
    -

Many thanks to Catalin and Evgenii.

Regards, Macpaul Lin