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

From: Macpaul Lin <macpaul.lin@mediatek.com>
To: Evgenii Stepanov <eugenis@google.com>
Cc: Sasha Levin <sashal@kernel.org>, Peter Chen <peter.chen@nxp.com>,
	Vincent Pelletier <plr.vincent@gmail.com>,
	CC Hwang <cc.hwang@mediatek.com>,
	Mediatek WSD Upstream <wsd_upstream@mediatek.com>,
	Shen Jing <jingx.shen@intel.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jerry Zhang <zhangjerry@google.com>,
	Loda Chou <loda.chou@mediatek.com>,
	LKML <linux-kernel@vger.kernel.org>,
	stable@vger.kernel.org,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	Miles Chen <miles.chen@mediatek.com>,
	linux-usb@vger.kernel.org, John Stultz <john.stultz@linaro.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrey Konovalov <andreyknvl@google.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-mediatek@lists.infradead.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4] usb: gadget: f_fs: try to fix AIO issue under ARM 64 bit TAGGED mode
Date: Wed, 4 Mar 2020 10:42:23 +0800	[thread overview]
Message-ID: <1583289743.12083.52.camel@mtkswgap22> (raw)
In-Reply-To: <CAFKCwrj-0aQN_cUxf8=h7AbfS_rLEwxqePZN2kGHZxgWi2=ncw@mail.gmail.com>

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel