[RFC] ARC: mm: Restrict definition of pfn_valid() macro for CONFIG_FLATMEM

[RFC] ARC: mm: Restrict definition of pfn_valid() macro for CONFIG_FLATMEM

[Yuriy Kolerov]

Despite the fact that subtraction of unsigned integers is a defined
behaviour however such operations can lead to unexpected results. Thus
it is better to check both left and right boundaries to avoid potential
bugs as it done in the generic page.h.

Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com>
---
 arch/arc/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arc/include/asm/page.h b/arch/arc/include/asm/page.h
index 296c342..81cfc6c7 100644
--- a/arch/arc/include/asm/page.h
+++ b/arch/arc/include/asm/page.h
@@ -88,7 +88,7 @@ typedef pte_t * pgtable_t;
 #define ARCH_PFN_OFFSET		virt_to_pfn(CONFIG_LINUX_LINK_BASE)
 
 #ifdef CONFIG_FLATMEM
-#define pfn_valid(pfn)		(((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
+#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
 #endif
 
 /*
-- 
2.7.4

Re: [RFC] ARC: mm: Restrict definition of pfn_valid() macro for CONFIG_FLATMEM

[Michal Hocko]

On Tue 29-11-16 18:29:06, Yuriy Kolerov wrote:
> Despite the fact that subtraction of unsigned integers is a defined
> behaviour however such operations can lead to unexpected results. Thus
> it is better to check both left and right boundaries to avoid potential
> bugs as it done in the generic page.h.

Why and which code would use an out of range pfn? Why other arches do
not need to care?

> 
> Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com>
> ---
>  arch/arc/include/asm/page.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arc/include/asm/page.h b/arch/arc/include/asm/page.h
> index 296c342..81cfc6c7 100644
> --- a/arch/arc/include/asm/page.h
> +++ b/arch/arc/include/asm/page.h
> @@ -88,7 +88,7 @@ typedef pte_t * pgtable_t;
>  #define ARCH_PFN_OFFSET		virt_to_pfn(CONFIG_LINUX_LINK_BASE)
>  
>  #ifdef CONFIG_FLATMEM
> -#define pfn_valid(pfn)		(((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
> +#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
>  #endif
>  
>  /*
> -- 
> 2.7.4
> 

-- 
Michal Hocko
SUSE Labs

RE: [RFC] ARC: mm: Restrict definition of pfn_valid() macro for CONFIG_FLATMEM

[Yuriy Kolerov]

> -----Original Message-----
> From: Michal Hocko [mailto:mhocko@kernel.org]
> Sent: Wednesday, November 30, 2016 12:17 PM
> To: Yuriy Kolerov <yuriy.kolerov@synopsys.com>
> Cc: linux-snps-arc@lists.infradead.org; Vineet.Gupta1@synopsys.com;
> Alexey.Brodkin@synopsys.com; linux-kernel@vger.kernel.org
> Subject: Re: [RFC] ARC: mm: Restrict definition of pfn_valid() macro for
> CONFIG_FLATMEM
> 
> On Tue 29-11-16 18:29:06, Yuriy Kolerov wrote:
> > Despite the fact that subtraction of unsigned integers is a defined
> > behaviour however such operations can lead to unexpected results. Thus
> > it is better to check both left and right boundaries to avoid
> > potential bugs as it done in the generic page.h.
> 
> Why and which code would use an out of range pfn? Why other arches do
> not need to care?

Actually some arches do care about checking of both left and right boundaries (e.g. avr32, sparc, etc). The problem is that a value of pfn may be calculated incorrectly in some places of the kernel. E.g. not long ago I sent a patch which fixes truncation of the most significant byte in pfn/pte in some cases (in the kernel with PAE40, however it is not a FLATMEM case). So such situations can happens in the most unexpected places.

> >
> > Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com>
> > ---
> >  arch/arc/include/asm/page.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arc/include/asm/page.h b/arch/arc/include/asm/page.h
> > index 296c342..81cfc6c7 100644
> > --- a/arch/arc/include/asm/page.h
> > +++ b/arch/arc/include/asm/page.h
> > @@ -88,7 +88,7 @@ typedef pte_t * pgtable_t;
> >  #define ARCH_PFN_OFFSET
> 	virt_to_pfn(CONFIG_LINUX_LINK_BASE)
> >
> >  #ifdef CONFIG_FLATMEM
> > -#define pfn_valid(pfn)		(((pfn) - ARCH_PFN_OFFSET) <
> max_mapnr)
> > +#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET &&
> ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
> >  #endif
> >
> >  /*
> > --
> > 2.7.4
> >
> 
> --
> Michal Hocko
> SUSE Labs

Re: [RFC] ARC: mm: Restrict definition of pfn_valid() macro for CONFIG_FLATMEM

[Vineet Gupta]

On 11/30/2016 06:21 AM, Yuriy Kolerov wrote:
>> On Tue 29-11-16 18:29:06, Yuriy Kolerov wrote:
>>> > > Despite the fact that subtraction of unsigned integers is a defined
>>> > > behaviour however such operations can lead to unexpected results. Thus
>>> > > it is better to check both left and right boundaries to avoid
>>> > > potential bugs as it done in the generic page.h.
>> > 
>> > Why and which code would use an out of range pfn? Why other arches do
>> > not need to care?
> Actually some arches do care about checking of both left and right boundaries (e.g. avr32, sparc, etc). The problem is that a value of pfn may be calculated incorrectly in some places of the kernel. E.g. not long ago I sent a patch which fixes truncation of the most significant byte in pfn/pte in some cases (in the kernel with PAE40, however it is not a FLATMEM case). So such situations can happens in the most unexpected places.
> 

So the point is - is this a preventive fix (desired thing) or it being there would
have helped find the PAE40 bug earlier / easier. Woudl it have prevented the
kernel crash. If so then this is a nobrainer fix.

BTW did you try to gauge the code gen impact - this function gets pulled all over
the place in mm code. So build kernel with and w/o change and do a
scripts/bloat-o-meter

-Vineet

RE: [RFC] ARC: mm: Restrict definition of pfn_valid() macro for CONFIG_FLATMEM

[Yuriy Kolerov]

> -----Original Message-----
> From: Vineet Gupta [mailto:vgupta@synopsys.com]
> Sent: Wednesday, November 30, 2016 7:55 PM
> To: Yuriy Kolerov <Yuriy.Kolerov@synopsys.com>; Michal Hocko
> <mhocko@kernel.org>
> Cc: linux-snps-arc@lists.infradead.org; Alexey.Brodkin@synopsys.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [RFC] ARC: mm: Restrict definition of pfn_valid() macro for
> CONFIG_FLATMEM
> 
> On 11/30/2016 06:21 AM, Yuriy Kolerov wrote:
> >> On Tue 29-11-16 18:29:06, Yuriy Kolerov wrote:
> >>> > > Despite the fact that subtraction of unsigned integers is a
> >>> > > defined behaviour however such operations can lead to unexpected
> >>> > > results. Thus it is better to check both left and right
> >>> > > boundaries to avoid potential bugs as it done in the generic page.h.
> >> >
> >> > Why and which code would use an out of range pfn? Why other arches
> >> > do not need to care?
> > Actually some arches do care about checking of both left and right
> boundaries (e.g. avr32, sparc, etc). The problem is that a value of pfn may be
> calculated incorrectly in some places of the kernel. E.g. not long ago I sent a
> patch which fixes truncation of the most significant byte in pfn/pte in some
> cases (in the kernel with PAE40, however it is not a FLATMEM case). So such
> situations can happens in the most unexpected places.
> >
> 
> So the point is - is this a preventive fix (desired thing) or it being there would
> have helped find the PAE40 bug earlier / easier. Woudl it have prevented the
> kernel crash. If so then this is a nobrainer fix.

This fix can help to find bugs which are related to wrong pfn values and only for FLATMEM case (usually when PAE40 is turned off). However I have just figured out that it is impossible to pass such bad unsigned pfn value which passes pfn_valid() check (usually the kernel passes a value from unsigned long variable)... This fix may help in cases when the kernel accidently passes a signed long value as pfn to the macro. Frankly speaking there are low chances that such thing can ever happen so I'm a little paranoid about it.

> BTW did you try to gauge the code gen impact - this function gets pulled all
> over the place in mm code. So build kernel with and w/o change and do a
> scripts/bloat-o-meter

Report from that script (extra 112 bytes):

add/remove: 0/0 grow/shrink: 9/1 up/down: 122/-10 (112)
function                                     old     new   delta
set_zone_contiguous                          212     248     +36
__pageblock_pfn_to_page                      120     136     +16
vm_insert_pfn_prot                           432     444     +12
vm_insert_pfn                                436     448     +12
kpagecount_read                              360     372     +12
reserve_bootmem_region                       110     120     +10
memremap                                     248     256      +8
kpageflags_read                              840     848      +8
devm_memremap                                356     364      +8
pagetypeinfo_show                            752     742     -10
Total: Before=3785631, After=3785743, chg +0.00%

> -Vineet