| Message ID | 44e5738a584c11801b2b8f1231898918efc8634a.1643047180.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan, vmalloc, arm64: add vmalloc tagging support for SW/HW_TAGS | expand |
On 1/24/22 19:05, andrey.konovalov@linux.dev wrote: > From: Andrey Konovalov <andreyknvl@google.com> > > Only define the ___GFP_SKIP_KASAN_POISON flag when CONFIG_KASAN_HW_TAGS > is enabled. > > This patch it not useful by itself, but it prepares the code for > additions of new KASAN-specific GFP patches. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > Changes v3->v4: > - This is a new patch. > --- > include/linux/gfp.h | 8 +++++++- > include/trace/events/mmflags.h | 12 +++++++++--- > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index 581a1f47b8a2..96f707931770 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -54,7 +54,11 @@ struct vm_area_struct; > #define ___GFP_THISNODE 0x200000u > #define ___GFP_ACCOUNT 0x400000u > #define ___GFP_ZEROTAGS 0x800000u > +#ifdef CONFIG_KASAN_HW_TAGS > #define ___GFP_SKIP_KASAN_POISON 0x1000000u > +#else > +#define ___GFP_SKIP_KASAN_POISON 0 > +#endif > #ifdef CONFIG_LOCKDEP > #define ___GFP_NOLOCKDEP 0x2000000u > #else > @@ -251,7 +255,9 @@ struct vm_area_struct; > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) > > /* Room for N __GFP_FOO bits */ > -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) > +#define __GFP_BITS_SHIFT (24 + \ > + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ > + IS_ENABLED(CONFIG_LOCKDEP)) This breaks __GFP_NOLOCKDEP, see: https://lore.kernel.org/all/YjoJ4CzB3yfWSV1F@linutronix.de/ > #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) > > /** > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h > index 116ed4d5d0f8..cb4520374e2c 100644 > --- a/include/trace/events/mmflags.h > +++ b/include/trace/events/mmflags.h > @@ -49,12 +49,18 @@ > {(unsigned long)__GFP_RECLAIM, "__GFP_RECLAIM"}, \ > {(unsigned long)__GFP_DIRECT_RECLAIM, "__GFP_DIRECT_RECLAIM"},\ > {(unsigned long)__GFP_KSWAPD_RECLAIM, "__GFP_KSWAPD_RECLAIM"},\ > - {(unsigned long)__GFP_ZEROTAGS, "__GFP_ZEROTAGS"}, \ > - {(unsigned long)__GFP_SKIP_KASAN_POISON,"__GFP_SKIP_KASAN_POISON"}\ > + {(unsigned long)__GFP_ZEROTAGS, "__GFP_ZEROTAGS"} \ > + > +#ifdef CONFIG_KASAN_HW_TAGS > +#define __def_gfpflag_names_kasan \ > + , {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} > +#else > +#define __def_gfpflag_names_kasan > +#endif > > #define show_gfp_flags(flags) \ > (flags) ? __print_flags(flags, "|", \ > - __def_gfpflag_names \ > + __def_gfpflag_names __def_gfpflag_names_kasan \ > ) : "none" > > #ifdef CONFIG_MMU
On 2022-03-23 12:48:29 [+0100], Vlastimil Babka wrote: > > +#ifdef CONFIG_KASAN_HW_TAGS > > #define ___GFP_SKIP_KASAN_POISON 0x1000000u > > +#else > > +#define ___GFP_SKIP_KASAN_POISON 0 > > +#endif > > #ifdef CONFIG_LOCKDEP > > #define ___GFP_NOLOCKDEP 0x2000000u > > #else > > @@ -251,7 +255,9 @@ struct vm_area_struct; > > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) > > > > /* Room for N __GFP_FOO bits */ > > -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) > > +#define __GFP_BITS_SHIFT (24 + \ > > + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ > > + IS_ENABLED(CONFIG_LOCKDEP)) > > This breaks __GFP_NOLOCKDEP, see: > https://lore.kernel.org/all/YjoJ4CzB3yfWSV1F@linutronix.de/ This could work because ___GFP_NOLOCKDEP is still 0x2000000u. In ("kasan, page_alloc: allow skipping memory init for HW_TAGS") https://lore.kernel.org/all/0d53efeff345de7d708e0baa0d8829167772521e.1643047180.git.andreyknvl@google.com/ This is replaced with 0x8000000u which breaks lockdep. Sebastian
On 3/23/22 14:02, Sebastian Andrzej Siewior wrote: > On 2022-03-23 12:48:29 [+0100], Vlastimil Babka wrote: >>> +#ifdef CONFIG_KASAN_HW_TAGS >>> #define ___GFP_SKIP_KASAN_POISON 0x1000000u >>> +#else >>> +#define ___GFP_SKIP_KASAN_POISON 0 >>> +#endif >>> #ifdef CONFIG_LOCKDEP >>> #define ___GFP_NOLOCKDEP 0x2000000u >>> #else >>> @@ -251,7 +255,9 @@ struct vm_area_struct; >>> #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) >>> >>> /* Room for N __GFP_FOO bits */ >>> -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) >>> +#define __GFP_BITS_SHIFT (24 + \ >>> + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ >>> + IS_ENABLED(CONFIG_LOCKDEP)) >> >> This breaks __GFP_NOLOCKDEP, see: >> https://lore.kernel.org/all/YjoJ4CzB3yfWSV1F@linutronix.de/ > > This could work because ___GFP_NOLOCKDEP is still 0x2000000u. In Hm but already this patch makes gfp_allowed_mask to be 0x1ffffff (thus not covering 0x2000000u) when CONFIG_LOCKDEP is enabled and the KASAN stuff not? 0x8000000u is just even further away. > ("kasan, page_alloc: allow skipping memory init for HW_TAGS") > https://lore.kernel.org/all/0d53efeff345de7d708e0baa0d8829167772521e.1643047180.git.andreyknvl@google.com/ > > This is replaced with 0x8000000u which breaks lockdep. > > Sebastian >
On Wed, Mar 23, 2022 at 2:02 PM Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > On 2022-03-23 12:48:29 [+0100], Vlastimil Babka wrote: > > > +#ifdef CONFIG_KASAN_HW_TAGS > > > #define ___GFP_SKIP_KASAN_POISON 0x1000000u > > > +#else > > > +#define ___GFP_SKIP_KASAN_POISON 0 > > > +#endif > > > #ifdef CONFIG_LOCKDEP > > > #define ___GFP_NOLOCKDEP 0x2000000u > > > #else > > > @@ -251,7 +255,9 @@ struct vm_area_struct; > > > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) > > > > > > /* Room for N __GFP_FOO bits */ > > > -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) > > > +#define __GFP_BITS_SHIFT (24 + \ > > > + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ > > > + IS_ENABLED(CONFIG_LOCKDEP)) > > > > This breaks __GFP_NOLOCKDEP, see: > > https://lore.kernel.org/all/YjoJ4CzB3yfWSV1F@linutronix.de/ > > This could work because ___GFP_NOLOCKDEP is still 0x2000000u. In > ("kasan, page_alloc: allow skipping memory init for HW_TAGS") > https://lore.kernel.org/all/0d53efeff345de7d708e0baa0d8829167772521e.1643047180.git.andreyknvl@google.com/ > > This is replaced with 0x8000000u which breaks lockdep. > > Sebastian Hi Sebastian, Indeed, sorry for breaking lockdep. Thank you for the report! I wonder what's the proper fix for this. Perhaps, don't hide KASAN GFP bits under CONFIG_KASAN_HW_TAGS? And then do: #define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP)) Vlastimil, Andrew do you have any preference? If my suggestion sounds good, Andrew, could you directly apply the changes? They are needed for these 3 patches: kasan, page_alloc: allow skipping memory init for HW_TAGS kasan, page_alloc: allow skipping unpoisoning for HW_TAGS kasan, mm: only define ___GFP_SKIP_KASAN_POISON with HW_TAGS As these depend on each other, I can't send separate patches that can be folded for all 3. Thanks!
On 3/23/22 14:36, Andrey Konovalov wrote: > On Wed, Mar 23, 2022 at 2:02 PM Sebastian Andrzej Siewior > <bigeasy@linutronix.de> wrote: >> >> On 2022-03-23 12:48:29 [+0100], Vlastimil Babka wrote: >>>> +#ifdef CONFIG_KASAN_HW_TAGS >>>> #define ___GFP_SKIP_KASAN_POISON 0x1000000u >>>> +#else >>>> +#define ___GFP_SKIP_KASAN_POISON 0 >>>> +#endif >>>> #ifdef CONFIG_LOCKDEP >>>> #define ___GFP_NOLOCKDEP 0x2000000u >>>> #else >>>> @@ -251,7 +255,9 @@ struct vm_area_struct; >>>> #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) >>>> >>>> /* Room for N __GFP_FOO bits */ >>>> -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) >>>> +#define __GFP_BITS_SHIFT (24 + \ >>>> + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ >>>> + IS_ENABLED(CONFIG_LOCKDEP)) >>> >>> This breaks __GFP_NOLOCKDEP, see: >>> https://lore.kernel.org/all/YjoJ4CzB3yfWSV1F@linutronix.de/ >> >> This could work because ___GFP_NOLOCKDEP is still 0x2000000u. In >> ("kasan, page_alloc: allow skipping memory init for HW_TAGS") >> https://lore.kernel.org/all/0d53efeff345de7d708e0baa0d8829167772521e.1643047180.git.andreyknvl@google.com/ >> >> This is replaced with 0x8000000u which breaks lockdep. >> >> Sebastian > > Hi Sebastian, > > Indeed, sorry for breaking lockdep. Thank you for the report! > > I wonder what's the proper fix for this. Perhaps, don't hide KASAN GFP > bits under CONFIG_KASAN_HW_TAGS? And then do: > > #define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP)) > > Vlastimil, Andrew do you have any preference? I guess it's the simplest thing to do for now. For the future we can still improve and handle all combinations of kasan/lockdep to occupy as few bits as possible and set the shift/mask appropriately. Or consider first if it's necessary anyway. I don't know if we really expect at any point to start triggering the BUILD_BUG_ON() in radix_tree_init() and then only some combination of configs will reduce the flags to a number that works. Or is there anything else that depends on __GFP_BITS_SHIFT? I mean if we don't expect to go this way, we can just define __GFP_BITS_SHIFT as a constant that assumes all the config-dependent flags to be defined (not zero). > If my suggestion sounds good, Andrew, could you directly apply the > changes? They are needed for these 3 patches: > > kasan, page_alloc: allow skipping memory init for HW_TAGS > kasan, page_alloc: allow skipping unpoisoning for HW_TAGS > kasan, mm: only define ___GFP_SKIP_KASAN_POISON with HW_TAGS > > As these depend on each other, I can't send separate patches that can > be folded for all 3. > > Thanks!
On Wed, Mar 23, 2022 at 02:57:30PM +0100, Vlastimil Babka wrote: > I guess it's the simplest thing to do for now. For the future we can > still improve and handle all combinations of kasan/lockdep to occupy as > few bits as possible and set the shift/mask appropriately. Or consider > first if it's necessary anyway. I don't know if we really expect at any > point to start triggering the BUILD_BUG_ON() in radix_tree_init() and > then only some combination of configs will reduce the flags to a number > that works. Or is there anything else that depends on __GFP_BITS_SHIFT? The correct long-term solution is to transition all the radix tree users to the XArray, which has the GFP flags specified in the correct place (ie at the call site) instead of embedding the GFP flags in the data structure. I've paused work on that while I work on folios; by my count there are about 60 users left. What I really need is something which prevents any attempt to add new users. Maybe that's a job for checkpatch.
On Wed, 23 Mar 2022 14:36:29 +0100 Andrey Konovalov <andreyknvl@gmail.com> wrote: > If my suggestion sounds good, Andrew, could you directly apply the > changes? They are needed for these 3 patches: > > kasan, page_alloc: allow skipping memory init for HW_TAGS > kasan, page_alloc: allow skipping unpoisoning for HW_TAGS > kasan, mm: only define ___GFP_SKIP_KASAN_POISON with HW_TAGS > > As these depend on each other, I can't send separate patches that can > be folded for all 3. It's all upstream now, so please send along a fixup patch.
diff --git a/include/linux/gfp.h b/include/linux/gfp.h index 581a1f47b8a2..96f707931770 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -54,7 +54,11 @@ struct vm_area_struct; #define ___GFP_THISNODE 0x200000u #define ___GFP_ACCOUNT 0x400000u #define ___GFP_ZEROTAGS 0x800000u +#ifdef CONFIG_KASAN_HW_TAGS #define ___GFP_SKIP_KASAN_POISON 0x1000000u +#else +#define ___GFP_SKIP_KASAN_POISON 0 +#endif #ifdef CONFIG_LOCKDEP #define ___GFP_NOLOCKDEP 0x2000000u #else @@ -251,7 +255,9 @@ struct vm_area_struct; #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) /* Room for N __GFP_FOO bits */ -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) +#define __GFP_BITS_SHIFT (24 + \ + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ + IS_ENABLED(CONFIG_LOCKDEP)) #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) /** diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h index 116ed4d5d0f8..cb4520374e2c 100644 --- a/include/trace/events/mmflags.h +++ b/include/trace/events/mmflags.h @@ -49,12 +49,18 @@ {(unsigned long)__GFP_RECLAIM, "__GFP_RECLAIM"}, \ {(unsigned long)__GFP_DIRECT_RECLAIM, "__GFP_DIRECT_RECLAIM"},\ {(unsigned long)__GFP_KSWAPD_RECLAIM, "__GFP_KSWAPD_RECLAIM"},\ - {(unsigned long)__GFP_ZEROTAGS, "__GFP_ZEROTAGS"}, \ - {(unsigned long)__GFP_SKIP_KASAN_POISON,"__GFP_SKIP_KASAN_POISON"}\ + {(unsigned long)__GFP_ZEROTAGS, "__GFP_ZEROTAGS"} \ + +#ifdef CONFIG_KASAN_HW_TAGS +#define __def_gfpflag_names_kasan \ + , {(unsigned long)__GFP_SKIP_KASAN_POISON, "__GFP_SKIP_KASAN_POISON"} +#else +#define __def_gfpflag_names_kasan +#endif #define show_gfp_flags(flags) \ (flags) ? __print_flags(flags, "|", \ - __def_gfpflag_names \ + __def_gfpflag_names __def_gfpflag_names_kasan \ ) : "none" #ifdef CONFIG_MMU