| Message ID | 20191122112621.204798-7-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
On Fri, 22 Nov 2019 at 12:26, <glider@google.com> wrote: > > This flag is to be used by KMSAN runtime to mark that newly created > memory pages don't need KMSAN metadata backing them. > > Signed-off-by: Alexander Potapenko <glider@google.com> > To: Alexander Potapenko <glider@google.com> > Cc: Vegard Nossum <vegard.nossum@oracle.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: linux-mm@kvack.org > > --- > We can't decide what to do here: > - do we need to conditionally define ___GFP_NO_KMSAN_SHADOW depending on > CONFIG_KMSAN like LOCKDEP does? > - if KMSAN is defined, and LOCKDEP is not, do we want to "compactify" the GFP > bits? A maintainer would know the real answer, but this is my guess: making the behaviour not change without KMSAN would probably be better. It would require some ifdef trickery to deal with LOCKDEP on/off case though. > Change-Id: If5d0352fd5711ad103328e2c185eb885e826423a > --- > include/linux/gfp.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index fb07b503dc45..b4e7963cd94b 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -44,6 +44,7 @@ struct vm_area_struct; > #else > #define ___GFP_NOLOCKDEP 0 > #endif Since this change unconditionally changes GFP_BITS_SHIFT to 25, the #ifdef for GFP_NOLOCKDEP could also go away -- but: see above. > +#define ___GFP_NO_KMSAN_SHADOW 0x1000000u > /* If the above are modified, __GFP_BITS_SHIFT may need updating */ > > /* > @@ -212,12 +213,13 @@ struct vm_area_struct; > #define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN) > #define __GFP_COMP ((__force gfp_t)___GFP_COMP) > #define __GFP_ZERO ((__force gfp_t)___GFP_ZERO) > +#define __GFP_NO_KMSAN_SHADOW ((__force gfp_t)___GFP_NO_KMSAN_SHADOW) Should this be ordered after __GFP_NOLOCKDEP with a brief comment what it does? All of these up to __GFP_ZERO have a doc comment above. > /* Disable lockdep for GFP context tracking */ > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) > > /* Room for N __GFP_FOO bits */ > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP)) > +#define __GFP_BITS_SHIFT (25) > #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) > > /** > -- > 2.24.0.432.g9d3f5f5b63-goog >
On Wed, Nov 27, 2019 at 3:48 PM Marco Elver <elver@google.com> wrote: > > On Fri, 22 Nov 2019 at 12:26, <glider@google.com> wrote: > > > > This flag is to be used by KMSAN runtime to mark that newly created > > memory pages don't need KMSAN metadata backing them. > > > > Signed-off-by: Alexander Potapenko <glider@google.com> > > To: Alexander Potapenko <glider@google.com> > > Cc: Vegard Nossum <vegard.nossum@oracle.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Cc: linux-mm@kvack.org > > > > --- > > We can't decide what to do here: > > - do we need to conditionally define ___GFP_NO_KMSAN_SHADOW depending on > > CONFIG_KMSAN like LOCKDEP does? > > - if KMSAN is defined, and LOCKDEP is not, do we want to "compactify" the GFP > > bits? > > A maintainer would know the real answer, but this is my guess: making > the behaviour not change without KMSAN would probably be better. It > would require some ifdef trickery to deal with LOCKDEP on/off case > though. I actually find it quite unfortunate that LOCKDEP has this on/off case. It's already inconvenient to add a new GFP flag past ___GFP_NOLOCKDEP, as its value will be depending on this blinking LOCKDEP bit. Now imagine someone wants to add a second GFP flag that can be disabled similarly to LOCKDEP - there'll be even more hassle counting which bits are present. Michal, do you think it's possible to make __GFP_BITS_SHIFT independent from CONFIG_LOCKDEP? > > Change-Id: If5d0352fd5711ad103328e2c185eb885e826423a > > --- > > include/linux/gfp.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > > index fb07b503dc45..b4e7963cd94b 100644 > > --- a/include/linux/gfp.h > > +++ b/include/linux/gfp.h > > @@ -44,6 +44,7 @@ struct vm_area_struct; > > #else > > #define ___GFP_NOLOCKDEP 0 > > #endif > > Since this change unconditionally changes GFP_BITS_SHIFT to 25, the > #ifdef for GFP_NOLOCKDEP could also go away -- but: see above. > > > +#define ___GFP_NO_KMSAN_SHADOW 0x1000000u > > /* If the above are modified, __GFP_BITS_SHIFT may need updating */ > > > > /* > > @@ -212,12 +213,13 @@ struct vm_area_struct; > > #define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN) > > #define __GFP_COMP ((__force gfp_t)___GFP_COMP) > > #define __GFP_ZERO ((__force gfp_t)___GFP_ZERO) > > +#define __GFP_NO_KMSAN_SHADOW ((__force gfp_t)___GFP_NO_KMSAN_SHADOW) > > Should this be ordered after __GFP_NOLOCKDEP with a brief comment what > it does? All of these up to __GFP_ZERO have a doc comment above. > > > /* Disable lockdep for GFP context tracking */ > > #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) > > > > /* Room for N __GFP_FOO bits */ > > -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP)) > > +#define __GFP_BITS_SHIFT (25) > > #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) > > > > /** > > -- > > 2.24.0.432.g9d3f5f5b63-goog > >
diff --git a/include/linux/gfp.h b/include/linux/gfp.h index fb07b503dc45..b4e7963cd94b 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -44,6 +44,7 @@ struct vm_area_struct; #else #define ___GFP_NOLOCKDEP 0 #endif +#define ___GFP_NO_KMSAN_SHADOW 0x1000000u /* If the above are modified, __GFP_BITS_SHIFT may need updating */ /* @@ -212,12 +213,13 @@ struct vm_area_struct; #define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN) #define __GFP_COMP ((__force gfp_t)___GFP_COMP) #define __GFP_ZERO ((__force gfp_t)___GFP_ZERO) +#define __GFP_NO_KMSAN_SHADOW ((__force gfp_t)___GFP_NO_KMSAN_SHADOW) /* Disable lockdep for GFP context tracking */ #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) /* Room for N __GFP_FOO bits */ -#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP)) +#define __GFP_BITS_SHIFT (25) #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) /**
This flag is to be used by KMSAN runtime to mark that newly created memory pages don't need KMSAN metadata backing them. Signed-off-by: Alexander Potapenko <glider@google.com> To: Alexander Potapenko <glider@google.com> Cc: Vegard Nossum <vegard.nossum@oracle.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: linux-mm@kvack.org --- We can't decide what to do here: - do we need to conditionally define ___GFP_NO_KMSAN_SHADOW depending on CONFIG_KMSAN like LOCKDEP does? - if KMSAN is defined, and LOCKDEP is not, do we want to "compactify" the GFP bits? Change-Id: If5d0352fd5711ad103328e2c185eb885e826423a --- include/linux/gfp.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)