[PATCH 0/2] ANDROID: cfi:free old cfi shadow asynchronously

[PATCH 0/2] ANDROID: cfi:free old cfi shadow asynchronously

[Haibo Li]

Currenly, it uses synchronize_rcu() to wait old rcu reader to go away
in update_shadow.In embedded platform like ARM CA7X,
load_module blocks 40~50ms in update_shadow.
When there are more than one hundred kernel modules,
it blocks several seconds.

To accelerate load_module,change synchronize_rcu to call_rcu.

*** BLURB HERE ***

Haibo Li (2):
  ANDROID: cfi: enable sanitize for cfi.c
  ANDROID: cfi: free old cfi shadow asynchronously

 kernel/Makefile |  3 ---
 kernel/cfi.c    | 20 +++++++++++++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] ANDROID: cfi: enable sanitize for cfi.c

[Haibo Li]

currenly,cfi.c is excluded from cfi sanitize because of cfi handler.
The side effect is that we can not transfer function pointer to
other files which enable cfi sanitize.

Enable cfi sanitize for cfi.c and bypass cfi check for __cfi_slowpath_diag

Signed-off-by: Haibo Li <haibo.li@mediatek.com>
Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>
---
 kernel/Makefile | 3 ---
 kernel/cfi.c    | 8 +++++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/Makefile b/kernel/Makefile
index a7e1f49ab2b3..a997bef1a200 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -40,9 +40,6 @@ KCSAN_SANITIZE_kcov.o := n
 UBSAN_SANITIZE_kcov.o := n
 CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack) -fno-stack-protector
 
-# Don't instrument error handlers
-CFLAGS_REMOVE_cfi.o := $(CC_FLAGS_CFI)
-
 obj-y += sched/
 obj-y += locking/
 obj-y += power/
diff --git a/kernel/cfi.c b/kernel/cfi.c
index 08102d19ec15..456771c8e454 100644
--- a/kernel/cfi.c
+++ b/kernel/cfi.c
@@ -311,7 +311,7 @@ static inline cfi_check_fn find_check_fn(unsigned long ptr)
 	return fn;
 }
 
-void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag)
+static inline void __nocfi _run_cfi_check(u64 id, void *ptr, void *diag)
 {
 	cfi_check_fn fn = find_check_fn((unsigned long)ptr);
 
@@ -320,6 +320,12 @@ void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag)
 	else /* Don't allow unchecked modules */
 		handle_cfi_failure(ptr);
 }
+
+void __cfi_slowpath_diag(u64 id, void *ptr, void *diag)
+{
+	/*run cfi check without cfi sanitize to avoid calling cfi handler recursively*/
+	_run_cfi_check(id, ptr, diag);
+}
 EXPORT_SYMBOL(__cfi_slowpath_diag);
 
 #else /* !CONFIG_MODULES */
-- 
2.25.1

Re: [PATCH 1/2] ANDROID: cfi: enable sanitize for cfi.c

[Sami Tolvanen]

On Thu, Jun 30, 2022 at 2:47 AM Haibo Li <haibo.li@mediatek.com> wrote:
>
> currenly,cfi.c is excluded from cfi sanitize because of cfi handler.
> The side effect is that we can not transfer function pointer to
> other files which enable cfi sanitize.
>
> Enable cfi sanitize for cfi.c and bypass cfi check for __cfi_slowpath_diag
>
> Signed-off-by: Haibo Li <haibo.li@mediatek.com>
> Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>
> ---
>  kernel/Makefile | 3 ---
>  kernel/cfi.c    | 8 +++++++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/Makefile b/kernel/Makefile
> index a7e1f49ab2b3..a997bef1a200 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -40,9 +40,6 @@ KCSAN_SANITIZE_kcov.o := n
>  UBSAN_SANITIZE_kcov.o := n
>  CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack) -fno-stack-protector
>
> -# Don't instrument error handlers
> -CFLAGS_REMOVE_cfi.o := $(CC_FLAGS_CFI)
> -
>  obj-y += sched/
>  obj-y += locking/
>  obj-y += power/
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 08102d19ec15..456771c8e454 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -311,7 +311,7 @@ static inline cfi_check_fn find_check_fn(unsigned long ptr)
>         return fn;
>  }
>
> -void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag)
> +static inline void __nocfi _run_cfi_check(u64 id, void *ptr, void *diag)
>  {
>         cfi_check_fn fn = find_check_fn((unsigned long)ptr);
>
> @@ -320,6 +320,12 @@ void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag)
>         else /* Don't allow unchecked modules */
>                 handle_cfi_failure(ptr);
>  }
> +
> +void __cfi_slowpath_diag(u64 id, void *ptr, void *diag)
> +{
> +       /*run cfi check without cfi sanitize to avoid calling cfi handler recursively*/
> +       _run_cfi_check(id, ptr, diag);
> +}
>  EXPORT_SYMBOL(__cfi_slowpath_diag);

You can just add __nocfi to __cfi_slowpath_diag, right? There's no
need for the separate function.

Sami

Re: [PATCH 1/2] ANDROID: cfi: enable sanitize for cfi.c

[Haibo Li]

> On Thu, Jun 30, 2022 at 2:47 AM Haibo Li <haibo.li@mediatek.com> wrote:
> >
> > currenly,cfi.c is excluded from cfi sanitize because of cfi handler.
> > The side effect is that we can not transfer function pointer to other
> > files which enable cfi sanitize.
> >
> > Enable cfi sanitize for cfi.c and bypass cfi check for
> > __cfi_slowpath_diag
> >
> > Signed-off-by: Haibo Li <haibo.li@mediatek.com>
> > Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>
> > ---
> >  kernel/Makefile | 3 ---
> >  kernel/cfi.c    | 8 +++++++-
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/Makefile b/kernel/Makefile index
> > a7e1f49ab2b3..a997bef1a200 100644
> > --- a/kernel/Makefile
> > +++ b/kernel/Makefile
> > @@ -40,9 +40,6 @@ KCSAN_SANITIZE_kcov.o := n
> UBSAN_SANITIZE_kcov.o :=
> > n  CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack)
> > -fno-stack-protector
> >
> > -# Don't instrument error handlers
> > -CFLAGS_REMOVE_cfi.o := $(CC_FLAGS_CFI)
> > -
> >  obj-y += sched/
> >  obj-y += locking/
> >  obj-y += power/
> > diff --git a/kernel/cfi.c b/kernel/cfi.c index
> > 08102d19ec15..456771c8e454 100644
> > --- a/kernel/cfi.c
> > +++ b/kernel/cfi.c
> > @@ -311,7 +311,7 @@ static inline cfi_check_fn find_check_fn(unsigned
> long ptr)
> >         return fn;
> >  }
> >
> > -void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag)
> > +static inline void __nocfi _run_cfi_check(u64 id, void *ptr, void
> > +*diag)
> >  {
> >         cfi_check_fn fn = find_check_fn((unsigned long)ptr);
> >
> > @@ -320,6 +320,12 @@ void __cfi_slowpath_diag(uint64_t id, void *ptr,
> void *diag)
> >         else /* Don't allow unchecked modules */
> >                 handle_cfi_failure(ptr);  }
> > +
> > +void __cfi_slowpath_diag(u64 id, void *ptr, void *diag) {
> > +       /*run cfi check without cfi sanitize to avoid calling cfi handler
> recursively*/
> > +       _run_cfi_check(id, ptr, diag); }
> >  EXPORT_SYMBOL(__cfi_slowpath_diag);
> 
> You can just add __nocfi to __cfi_slowpath_diag, right? There's no need for the
> separate function.

    You are right.Now there is no requirement for constant crc of __cfi_slowpath_diag.
I will change it later.
	

[PATCH 2/2] ANDROID: cfi: free old cfi shadow asynchronously

[Haibo Li]

Currenly, it uses synchronize_rcu() to wait old rcu reader to go away
in update_shadow.In embedded platform like ARM CA7X,
load_module blocks 40~50ms in update_shadow.
When there are more than one hundred kernel modules,
it blocks several seconds.

To accelerate load_module,change synchronize_rcu to call_rcu.

Signed-off-by: Haibo Li <haibo.li@mediatek.com>
Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>
---
 kernel/cfi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/cfi.c b/kernel/cfi.c
index 456771c8e454..a4836d59ca27 100644
--- a/kernel/cfi.c
+++ b/kernel/cfi.c
@@ -43,6 +43,8 @@ typedef u16 shadow_t;
 struct cfi_shadow {
 	/* Page index for the beginning of the shadow */
 	unsigned long base;
+	/* rcu to free old cfi_shadow asynchronously */
+	struct rcu_head rcu;
 	/* An array of __cfi_check locations (as indices to the shadow) */
 	shadow_t shadow[1];
 } __packed;
@@ -182,6 +184,13 @@ static void remove_module_from_shadow(struct cfi_shadow *s, struct module *mod,
 	}
 }
 
+static void _cfi_shadow_free_rcu(struct rcu_head *rcu)
+{
+	struct cfi_shadow *old = container_of(rcu, struct cfi_shadow, rcu);
+
+	vfree(old);
+}
+
 typedef void (*update_shadow_fn)(struct cfi_shadow *, struct module *,
 			unsigned long min_addr, unsigned long max_addr);
 
@@ -211,11 +220,10 @@ static void update_shadow(struct module *mod, unsigned long base_addr,
 
 	rcu_assign_pointer(cfi_shadow, next);
 	mutex_unlock(&shadow_update_lock);
-	synchronize_rcu();
 
 	if (prev) {
 		set_memory_rw((unsigned long)prev, SHADOW_PAGES);
-		vfree(prev);
+		call_rcu(&prev->rcu, _cfi_shadow_free_rcu);
 	}
 }
 
-- 
2.25.1

Re: [PATCH 2/2] ANDROID: cfi: free old cfi shadow asynchronously

[Sami Tolvanen]

Hi,

On Thu, Jun 30, 2022 at 2:47 AM Haibo Li <haibo.li@mediatek.com> wrote:
>
> Currenly, it uses synchronize_rcu() to wait old rcu reader to go away
> in update_shadow.In embedded platform like ARM CA7X,
> load_module blocks 40~50ms in update_shadow.
> When there are more than one hundred kernel modules,
> it blocks several seconds.
>
> To accelerate load_module,change synchronize_rcu to call_rcu.
>
> Signed-off-by: Haibo Li <haibo.li@mediatek.com>
> Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>

Thanks for the patch! Please drop ANDROID: from the subject line,
that's only used in the Android kernel trees.

>  kernel/cfi.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 456771c8e454..a4836d59ca27 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -43,6 +43,8 @@ typedef u16 shadow_t;
>  struct cfi_shadow {
>         /* Page index for the beginning of the shadow */
>         unsigned long base;
> +       /* rcu to free old cfi_shadow asynchronously */
> +       struct rcu_head rcu;
>         /* An array of __cfi_check locations (as indices to the shadow) */
>         shadow_t shadow[1];
>  } __packed;
> @@ -182,6 +184,13 @@ static void remove_module_from_shadow(struct cfi_shadow *s, struct module *mod,
>         }
>  }
>
> +static void _cfi_shadow_free_rcu(struct rcu_head *rcu)

I think this can be simply renamed to free_shadow.

> +{
> +       struct cfi_shadow *old = container_of(rcu, struct cfi_shadow, rcu);
> +
> +       vfree(old);
> +}
> +
>  typedef void (*update_shadow_fn)(struct cfi_shadow *, struct module *,
>                         unsigned long min_addr, unsigned long max_addr);
>
> @@ -211,11 +220,10 @@ static void update_shadow(struct module *mod, unsigned long base_addr,
>
>         rcu_assign_pointer(cfi_shadow, next);
>         mutex_unlock(&shadow_update_lock);
> -       synchronize_rcu();
>
>         if (prev) {
>                 set_memory_rw((unsigned long)prev, SHADOW_PAGES);
> -               vfree(prev);
> +               call_rcu(&prev->rcu, _cfi_shadow_free_rcu);
>         }
>  }

It's probably better to keep the pages read-only until they're
actually released. Can you move the set_memory_rw call to the new
function?

Sami

Re: [PATCH 2/2] ANDROID: cfi: free old cfi shadow asynchronously

[Haibo Li]

Re: [PATCH 2/2] ANDROID: cfi: free old cfi shadow asynchronously

[Haibo Li]

> Hi,
> 
> On Thu, Jun 30, 2022 at 2:47 AM Haibo Li <haibo.li@mediatek.com> wrote:
> >
> > Currenly, it uses synchronize_rcu() to wait old rcu reader to go away
> > in update_shadow.In embedded platform like ARM CA7X, load_module
> > blocks 40~50ms in update_shadow.
> > When there are more than one hundred kernel modules, it blocks several
> > seconds.
> >
> > To accelerate load_module,change synchronize_rcu to call_rcu.
> >
> > Signed-off-by: Haibo Li <haibo.li@mediatek.com>
> > Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>
> 
> Thanks for the patch! Please drop ANDROID: from the subject line, that's only
> used in the Android kernel trees.
> 
   Thanks for the comment.I will change it in next patch.  
> >  kernel/cfi.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/cfi.c b/kernel/cfi.c index
> > 456771c8e454..a4836d59ca27 100644
> > --- a/kernel/cfi.c
> > +++ b/kernel/cfi.c
> > @@ -43,6 +43,8 @@ typedef u16 shadow_t;  struct cfi_shadow {
> >         /* Page index for the beginning of the shadow */
> >         unsigned long base;
> > +       /* rcu to free old cfi_shadow asynchronously */
> > +       struct rcu_head rcu;
> >         /* An array of __cfi_check locations (as indices to the shadow) */
> >         shadow_t shadow[1];
> >  } __packed;
> > @@ -182,6 +184,13 @@ static void remove_module_from_shadow(struct
> cfi_shadow *s, struct module *mod,
> >         }
> >  }
> >
> > +static void _cfi_shadow_free_rcu(struct rcu_head *rcu)
> 
> I think this can be simply renamed to free_shadow.
   Thanks for the comment.I will change it in next patch.
> 
> > +{
> > +       struct cfi_shadow *old = container_of(rcu, struct cfi_shadow,
> > +rcu);
> > +
> > +       vfree(old);
> > +}
> > +
> >  typedef void (*update_shadow_fn)(struct cfi_shadow *, struct module *,
> >                         unsigned long min_addr, unsigned long
> > max_addr);
> >
> > @@ -211,11 +220,10 @@ static void update_shadow(struct module *mod,
> > unsigned long base_addr,
> >
> >         rcu_assign_pointer(cfi_shadow, next);
> >         mutex_unlock(&shadow_update_lock);
> > -       synchronize_rcu();
> >
> >         if (prev) {
> >                 set_memory_rw((unsigned long)prev, SHADOW_PAGES);
> > -               vfree(prev);
> > +               call_rcu(&prev->rcu, _cfi_shadow_free_rcu);
> >         }
> >  }
> 
> It's probably better to keep the pages read-only until they're actually released.
> Can you move the set_memory_rw call to the new function?
> 
   Since call_rcu and rcu callbacks change members in &prev->rcu,the old pages need to be rw at first.