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

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

[Macpaul Lin]

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
fisrt 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 gdaget'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 userspace 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>
---
Changes for v2:
  - Fix build error for 32-bit load. An #if defined(CONFIG_ARM64) still need
    for avoiding undeclared defines.

 drivers/usb/gadget/function/f_fs.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index ce1d023..728c260 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -35,6 +35,7 @@
 #include <linux/mmu_context.h>
 #include <linux/poll.h>
 #include <linux/eventfd.h>
+#include <linux/thread_info.h>
 
 #include "u_fs.h"
 #include "u_f.h"
@@ -826,6 +827,10 @@ static void ffs_user_copy_worker(struct work_struct *work)
 	if (io_data->read && ret > 0) {
 		mm_segment_t oldfs = get_fs();
 
+#if defined(CONFIG_ARM64)
+		if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI))
+			set_thread_flag(TIF_TAGGED_ADDR);
+#endif
 		set_fs(USER_DS);
 		use_mm(io_data->mm);
 		ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);
-- 
1.7.9.5
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Peter Chen]

On 20-02-23 23:49:07, 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
> fisrt read I/O control. If this reading has been failed, adbd will

%s/fisrt/first

> 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 gdaget's AIO

%s/gdaget/gadget

> 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 userspace 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>
> ---
> Changes for v2:
>   - Fix build error for 32-bit load. An #if defined(CONFIG_ARM64) still need
>     for avoiding undeclared defines.
> 
>  drivers/usb/gadget/function/f_fs.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index ce1d023..728c260 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -35,6 +35,7 @@
>  #include <linux/mmu_context.h>
>  #include <linux/poll.h>
>  #include <linux/eventfd.h>
> +#include <linux/thread_info.h>
>  
>  #include "u_fs.h"
>  #include "u_f.h"
> @@ -826,6 +827,10 @@ static void ffs_user_copy_worker(struct work_struct *work)
>  	if (io_data->read && ret > 0) {
>  		mm_segment_t oldfs = get_fs();
>  
> +#if defined(CONFIG_ARM64)
> +		if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI))
> +			set_thread_flag(TIF_TAGGED_ADDR);
> +#endif
>  		set_fs(USER_DS);
>  		use_mm(io_data->mm);
>  		ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);
> -- 
> 1.7.9.5

-- 

Thanks,
Peter Chen
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Macpaul Lin]

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 userspace 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>
---
Changes for v3:
  - Fix misspelling in commit message.

Changes for v2:
  - Fix build error for 32-bit load. An #if defined(CONFIG_ARM64) still need
    for avoiding undeclared defines.

 drivers/usb/gadget/function/f_fs.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index ce1d023..728c260 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -35,6 +35,7 @@
 #include <linux/mmu_context.h>
 #include <linux/poll.h>
 #include <linux/eventfd.h>
+#include <linux/thread_info.h>
 
 #include "u_fs.h"
 #include "u_f.h"
@@ -826,6 +827,10 @@ static void ffs_user_copy_worker(struct work_struct *work)
 	if (io_data->read && ret > 0) {
 		mm_segment_t oldfs = get_fs();
 
+#if defined(CONFIG_ARM64)
+		if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI))
+			set_thread_flag(TIF_TAGGED_ADDR);
+#endif
 		set_fs(USER_DS);
 		use_mm(io_data->mm);
 		ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);
-- 
1.7.9.5
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Miles Chen]

On Tue, 2020-02-25 at 18:41 +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 userspace 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>
> ---
> Changes for v3:
>   - Fix misspelling in commit message.

Could you say "thank you" to Peter for his comment and add 
"Cc: Peter Chen <peter.chen@nxp.com>" to this patch, please?

> 
> Changes for v2:
>   - Fix build error for 32-bit load. An #if defined(CONFIG_ARM64) still need
>     for avoiding undeclared defines.
> 
>  drivers/usb/gadget/function/f_fs.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index ce1d023..728c260 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -35,6 +35,7 @@
>  #include <linux/mmu_context.h>
>  #include <linux/poll.h>
>  #include <linux/eventfd.h>
> +#include <linux/thread_info.h>
>  
>  #include "u_fs.h"
>  #include "u_f.h"
> @@ -826,6 +827,10 @@ static void ffs_user_copy_worker(struct work_struct *work)
>  	if (io_data->read && ret > 0) {
>  		mm_segment_t oldfs = get_fs();
>  
> +#if defined(CONFIG_ARM64)
> +		if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI))
> +			set_thread_flag(TIF_TAGGED_ADDR);
> +#endif
>  		set_fs(USER_DS);
>  		use_mm(io_data->mm);
>  		ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Catalin Marinas]

On Tue, Feb 25, 2020 at 06:41:55PM +0800, Macpaul Lin wrote:
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index ce1d023..728c260 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -35,6 +35,7 @@
>  #include <linux/mmu_context.h>
>  #include <linux/poll.h>
>  #include <linux/eventfd.h>
> +#include <linux/thread_info.h>
>  
>  #include "u_fs.h"
>  #include "u_f.h"
> @@ -826,6 +827,10 @@ static void ffs_user_copy_worker(struct work_struct *work)
>  	if (io_data->read && ret > 0) {
>  		mm_segment_t oldfs = get_fs();
>  
> +#if defined(CONFIG_ARM64)
> +		if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI))
> +			set_thread_flag(TIF_TAGGED_ADDR);
> +#endif
>  		set_fs(USER_DS);
>  		use_mm(io_data->mm);
>  		ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);

I really don't think that's the correct fix. The TIF_TAGGED_ADDR is a
per-thread property and not really compatible with use_mm(). We've had
tagged pointers in arm64 user-space since day 0 and access_ok() would
have prevented them, so this config is not something new. For some
reason, adb now passes them to the kernel (presumably because user-space
makes more use of them). If you have strong reasons not to fix it in
adb, the next best thing may be to untag the addresses in the usb gadget
driver.

-- 
Catalin

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Macpaul Lin]

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.

Changes for v3:
  - Fix misspelling in commit message.
    Thanks for Peter's review.

Changes for v2:
  - Fix build error for 32-bit load. An #if defined(CONFIG_ARM64) still need
    for avoiding undeclared defines.

 drivers/usb/gadget/function/f_fs.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

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;
 
-- 
1.7.9.5
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Catalin Marinas]

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.

> 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.

-- 
Catalin

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Macpaul Lin]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Catalin Marinas]

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")?

-- 
Catalin

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Macpaul Lin]

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.

Anyway, I've found that to disable TAGGED address in adbd is possible by
this way and will report to Google and see how they think.

diff --git a/adb/daemon/main.cpp b/adb/daemon/main.cpp
index 9e02e89ab..b2f6f8e3f 100644
--- a/adb/daemon/main.cpp
+++ b/adb/daemon/main.cpp
@@ -317,6 +317,8 @@ int main(int argc, char** argv) {
     mallopt(M_DECAY_TIME, 1);
 #endif

+    prctl(PR_SET_TAGGED_ADDR_CTRL, ~PR_TAGGED_ADDR_ENABLE, 0, 0, 0);
+
     while (true) {
         static struct option opts[] = {
                 {"root_seclabel", required_argument, nullptr, 's'},

Many thanks!
Macpaul Lin
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Catalin Marinas]

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);
-

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Macpaul Lin]

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