| Message ID | 20200910134802.3160311-1-lenaptr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sched.h: drop in_ubsan field when UBSAN is in trap mode | expand |
On Thu, Sep 10, 2020 at 02:48:02PM +0100, Elena Petrova wrote: > in_ubsan field of task_struct is only used in lib/ubsan.c, which in its > turn is used only `ifneq ($(CONFIG_UBSAN_TRAP),y)`. > > Removing unnecessary field from a task_struct will help preserve the > ABI between vanilla and CONFIG_UBSAN_TRAP'ed kernels. In particular, > this will help enabling bounds sanitizer transparently for Android's > GKI. > > Signed-off-by: Elena Petrova <lenaptr@google.com> Acked-by: Kees Cook <keescook@chromium.org> (This should be CCed to akpm who has been taking most of the ubsan patches lately.)
On Thu, Sep 10, 2020 at 3:48 PM Elena Petrova <lenaptr@google.com> wrote: > in_ubsan field of task_struct is only used in lib/ubsan.c, which in its > turn is used only `ifneq ($(CONFIG_UBSAN_TRAP),y)`. > > Removing unnecessary field from a task_struct will help preserve the > ABI between vanilla and CONFIG_UBSAN_TRAP'ed kernels. In particular, > this will help enabling bounds sanitizer transparently for Android's > GKI. The diff looks reasonable to me, but I'm curious about the justification in the commit message: Is the intent here that you want to be able to build a module without CONFIG_UBSAN and load it into a kernel that is built with CONFIG_UBSAN? Or the inverse? Does this mean that in the future, gating new exported functions, or new struct fields, on CONFIG_UBSAN (independent of whether CONFIG_UBSAN_TRAP is set) will break Android? If you really want to do this, and using alternatives to patch out the ubsan instructions is not an option, I wonder whether it would be more reasonable to at least add a configuration where CONFIG_UBSAN is enabled but the compiler flag is not actually set. Then you could unconditionally build that android kernel and its modules with that config option, and wouldn't have to worry about structure size issues, dependencies on undefined symbols and so on.
Hi Jann, On Thu, 10 Sep 2020 at 20:35, Jann Horn <jannh@google.com> wrote: > > On Thu, Sep 10, 2020 at 3:48 PM Elena Petrova <lenaptr@google.com> wrote: > > in_ubsan field of task_struct is only used in lib/ubsan.c, which in its > > turn is used only `ifneq ($(CONFIG_UBSAN_TRAP),y)`. > > > > Removing unnecessary field from a task_struct will help preserve the > > ABI between vanilla and CONFIG_UBSAN_TRAP'ed kernels. In particular, > > this will help enabling bounds sanitizer transparently for Android's > > GKI. > > The diff looks reasonable to me, but I'm curious about the > justification in the commit message: > > Is the intent here that you want to be able to build a module without > CONFIG_UBSAN and load it into a kernel that is built with > CONFIG_UBSAN? Or the inverse? The former. But more precisely, with GKI Google gives a promise, that when certain GKI is released, i.e. at 4.19, its ABI will never ever change (or, perhaps only change with <next letter> Android release), so vendor modules could have an independent development lifecycle. And this patch, when backported, will help enable boundsan on kernels where ABI has already been frozen. > Does this mean that in the future, gating new exported functions, or > new struct fields, on CONFIG_UBSAN (independent of whether > CONFIG_UBSAN_TRAP is set) will break Android? I don't understand what you mean here, sorry. > If you really want to do this, and using alternatives to patch out the > ubsan instructions is not an option, I wonder whether it would be more > reasonable to at least add a configuration where CONFIG_UBSAN is > enabled but the compiler flag is not actually set. Then you could > unconditionally build that android kernel and its modules with that > config option, and wouldn't have to worry about structure size issues, > dependencies on undefined symbols and so on. Such setup might be confusing for developers. We were considering something similar: to keep the in_ubsan field regardless of the CONFIG_UBSAN option. But since non-trap mode is unlikely to be used on production devices due to size and performance overheads, I think it's better to just get rid of an unused field, rather than balloon task_struct. Cheers, *lenaptr
On Fri, Sep 11, 2020 at 5:15 PM Elena Petrova <lenaptr@google.com> wrote: > On Thu, 10 Sep 2020 at 20:35, Jann Horn <jannh@google.com> wrote: > > On Thu, Sep 10, 2020 at 3:48 PM Elena Petrova <lenaptr@google.com> wrote: > > > in_ubsan field of task_struct is only used in lib/ubsan.c, which in its > > > turn is used only `ifneq ($(CONFIG_UBSAN_TRAP),y)`. > > > > > > Removing unnecessary field from a task_struct will help preserve the > > > ABI between vanilla and CONFIG_UBSAN_TRAP'ed kernels. In particular, > > > this will help enabling bounds sanitizer transparently for Android's > > > GKI. > > > > The diff looks reasonable to me, but I'm curious about the > > justification in the commit message: > > > > Is the intent here that you want to be able to build a module without > > CONFIG_UBSAN and load it into a kernel that is built with > > CONFIG_UBSAN? Or the inverse? > > The former. But more precisely, with GKI Google gives a promise, that > when certain GKI is released, i.e. at 4.19, its ABI will never ever > change (or, perhaps only change with <next letter> Android release), Really? How does that work when a kernel update needs to add elements to existing structs that are part of that "ABI"? Especially when those structs have something at the end that's variable-length (like task_struct) or they're embedded in something else? Maybe you should've done something like BPF's CORE if you really want to do something like that, teaching the compiler to generate relocations for struct offsets... > so vendor modules could have an independent development lifecycle. And > this patch, when backported, will help enable boundsan on kernels > where ABI has already been frozen. > > > Does this mean that in the future, gating new exported functions, or > > new struct fields, on CONFIG_UBSAN (independent of whether > > CONFIG_UBSAN_TRAP is set) will break Android? > > I don't understand what you mean here, sorry. Let's assume that at a later point, someone wants to track for each process how many UBSAN errors that process has seen so far. And maybe at that point, we have error recovery support in trap mode. So that person sends a patch that, among other things, adds something like this to task_struct: #ifdef CONFIG_UBSAN unsigned int ubsan_errors_seen; #endif If that patch lands, ABI compatibility between UBSAN=y&&UBSAN_TRAP=y and UBSAN=n will break again. I believe that it should normally be possible to add stuff like #ifdef CONFIG_<something> <some field declaration> #endif to an existing kernel struct without breaking anything (outside UAPI headers and such). Your patch assumes that that won't happen for CONFIG_UBSAN. > > If you really want to do this, and using alternatives to patch out the > > ubsan instructions is not an option, I wonder whether it would be more > > reasonable to at least add a configuration where CONFIG_UBSAN is > > enabled but the compiler flag is not actually set. Then you could > > unconditionally build that android kernel and its modules with that > > config option, and wouldn't have to worry about structure size issues, > > dependencies on undefined symbols and so on. > > Such setup might be confusing for developers. Yeah, but I think that that's still cleaner than assuming that some normal kernel flag won't change struct layouts... Anyway, the diff itself looks reasonable to me (although I dislike the commit message), but don't be surprised if this "ABI" is broken again in the future. > We were considering > something similar: to keep the in_ubsan field regardless of the > CONFIG_UBSAN option. But since non-trap mode is unlikely to be used on > production devices due to size and performance overheads, I think it's > better to just get rid of an unused field, rather than balloon > task_struct. > > Cheers, > *lenaptr
diff --git a/include/linux/sched.h b/include/linux/sched.h index afe01e232935..5c7b8dec236e 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1008,7 +1008,7 @@ struct task_struct { struct held_lock held_locks[MAX_LOCK_DEPTH]; #endif -#ifdef CONFIG_UBSAN +#if defined(CONFIG_UBSAN) && !defined(CONFIG_UBSAN_TRAP) unsigned int in_ubsan; #endif
in_ubsan field of task_struct is only used in lib/ubsan.c, which in its turn is used only `ifneq ($(CONFIG_UBSAN_TRAP),y)`. Removing unnecessary field from a task_struct will help preserve the ABI between vanilla and CONFIG_UBSAN_TRAP'ed kernels. In particular, this will help enabling bounds sanitizer transparently for Android's GKI. Signed-off-by: Elena Petrova <lenaptr@google.com> --- include/linux/sched.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)