[PATCH 0/1] pgo: Fix allocate_node() handling of non-vmlinux nodes

[PATCH 0/1] pgo: Fix allocate_node() handling of non-vmlinux nodes

[Jarmo Tiitto]

Currently allocate_node() will reserve nodes even if *p
doesn't point into __llvm_prf_data_start - __llvm_prf_data_end
range.

This is wrong:

Any instrumented modules that are not part of vmlinux
(built-in) will "steal" available nodes away from vmlinux.
This can result in exhaustion of nodes for vmlinux
and less accurate profile data.

Either way, any profiling data generated by loaded modules, 
if any, is unusable from vmlinux.profraw.
So just filtter them out.

Jarmo Tiitto (1):
  pgo: Fix allocate_node() handling of non-vmlinux nodes.

 kernel/pgo/instrument.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.31.1

[PATCH 1/1] pgo: Fix allocate_node() handling of non-vmlinux nodes.

[Jarmo Tiitto]

Currently allocate_node() will reserve nodes even if *p
doesn't point into __llvm_prf_data_start - __llvm_prf_data_end
range.

Fix it by checking if p points into vmlinux range
and otherwise return NULL.

Signed-off-by: Jarmo Tiitto <jarmo.tiitto@gmail.com>
---
 kernel/pgo/instrument.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/pgo/instrument.c b/kernel/pgo/instrument.c
index 0e07ee1b17d9..9bca535dfa91 100644
--- a/kernel/pgo/instrument.c
+++ b/kernel/pgo/instrument.c
@@ -55,6 +55,10 @@ void prf_unlock(unsigned long flags)
 static struct llvm_prf_value_node *allocate_node(struct llvm_prf_data *p,
 						 u32 index, u64 value)
 {
+	/* check if p points into vmlinux. If not, don't allocate. */
+	if (p < __llvm_prf_data_start || p >= __llvm_prf_data_end)
+		return NULL;
+
 	if (&__llvm_prf_vnds_start[current_node + 1] >= __llvm_prf_vnds_end)
 		return NULL; /* Out of nodes */
 
-- 
2.31.1

Re: [PATCH 1/1] pgo: Fix allocate_node() handling of non-vmlinux nodes.

[Kees Cook]

On Wed, Jun 02, 2021 at 03:57:04AM +0300, Jarmo Tiitto wrote:
> Currently allocate_node() will reserve nodes even if *p
> doesn't point into __llvm_prf_data_start - __llvm_prf_data_end
> range.
> 
> Fix it by checking if p points into vmlinux range
> and otherwise return NULL.
> 
> Signed-off-by: Jarmo Tiitto <jarmo.tiitto@gmail.com>
> ---
>  kernel/pgo/instrument.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/pgo/instrument.c b/kernel/pgo/instrument.c
> index 0e07ee1b17d9..9bca535dfa91 100644
> --- a/kernel/pgo/instrument.c
> +++ b/kernel/pgo/instrument.c
> @@ -55,6 +55,10 @@ void prf_unlock(unsigned long flags)
>  static struct llvm_prf_value_node *allocate_node(struct llvm_prf_data *p,
>  						 u32 index, u64 value)
>  {
> +	/* check if p points into vmlinux. If not, don't allocate. */
> +	if (p < __llvm_prf_data_start || p >= __llvm_prf_data_end)
> +		return NULL;

This should be a tighter check (struct llvm_prf_data has size, so just
checking for p < __llvm_prf_data_end isn't sufficient. I recommend using
the memory_contains() helper.

And I think this should be louder as it's entirely unexpected right
now. Perhaps:

	if (WARN_ON_ONCE(!memory_contains(__llvm_prf_data_start,
					  __llvm_prf_data_end,
					  p, sizeof(*p))))
		return NULL;

> +
>  	if (&__llvm_prf_vnds_start[current_node + 1] >= __llvm_prf_vnds_end)
>  		return NULL; /* Out of nodes */
>  
> -- 
> 2.31.1
> 

-- 
Kees Cook

Re: [PATCH 1/1] pgo: Fix allocate_node() handling of non-vmlinux nodes.

[jarmo.tiitto]

Kees Cook wrote keskiviikkona 2. kesäkuuta 2021 20.41.28 EEST:
> On Wed, Jun 02, 2021 at 03:57:04AM +0300, Jarmo Tiitto wrote:
> > Currently allocate_node() will reserve nodes even if *p
> > doesn't point into __llvm_prf_data_start - __llvm_prf_data_end
> > range.
> > 
> > Fix it by checking if p points into vmlinux range
> > and otherwise return NULL.
> > 
> > Signed-off-by: Jarmo Tiitto <jarmo.tiitto@gmail.com>
> > ---
> >  kernel/pgo/instrument.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/kernel/pgo/instrument.c b/kernel/pgo/instrument.c
> > index 0e07ee1b17d9..9bca535dfa91 100644
> > --- a/kernel/pgo/instrument.c
> > +++ b/kernel/pgo/instrument.c
> > @@ -55,6 +55,10 @@ void prf_unlock(unsigned long flags)
> >  static struct llvm_prf_value_node *allocate_node(struct llvm_prf_data *p,
> >  						 u32 index, u64 value)
> >  {
> > +	/* check if p points into vmlinux. If not, don't allocate. */
> > +	if (p < __llvm_prf_data_start || p >= __llvm_prf_data_end)
> > +		return NULL;
> 
> This should be a tighter check (struct llvm_prf_data has size, so just
> checking for p < __llvm_prf_data_end isn't sufficient. I recommend using
> the memory_contains() helper.
> 
> And I think this should be louder as it's entirely unexpected right
> now. Perhaps:
> 
> 	if (WARN_ON_ONCE(!memory_contains(__llvm_prf_data_start,
> 					  __llvm_prf_data_end,
> 					  p, sizeof(*p))))
> 		return NULL;
> 
> > +
> >  	if (&__llvm_prf_vnds_start[current_node + 1] >= __llvm_prf_vnds_end)
> >  		return NULL; /* Out of nodes */
> >  
> > -- 
> > 2.31.1
> > 
> 
> -- 
> Kees Cook
> 


Well, if you do that the WARN_ON_ONCE() will always trigger, unless CONFIG_MODULES=n 😇

This is because 'struct llvm_prf_data *p' argument is expected
(at least I think so) to point into __llvm_prf_data section in vmlinux
and also into each module's own __llvm_prf_data section.

So in the end the compiler supplied pointer is likely correct,
but the current v9 PGO patch attempts to reserve all vnodes
from vmlinux __llvm_prf_vnds section, instead of respective module section.

I think it would be normal to ignore pointers here,
until my module PGO machinery is ready.

But I agree on using memory_contains() for checking if p is within bounds.
I will follow on with v2 of this patch. :-)

-Jarmo