| Message ID | 1480433346-18054-1-git-send-email-yuriy.kolerov@synopsys.com |
|---|---|
| State | New |
| Headers | show |
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 >
> -----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
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
> -----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
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 /*
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(-)