Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event()

[Alexander Potapenko <glider@google.com>]

On Mon, Mar 2, 2020 at 5:57 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Mon, Mar 2, 2020 at 5:04 AM <glider@google.com> wrote:
> >
> > With CONFIG_INIT_STACK_ALL enabled, the local __wq_entry is initialized
> > twice. Because Clang is currently unable to optimize the automatic
> > initialization away (init_wait_entry() is defined in another translation
> > unit), remove it with the __no_initialize annotation.
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> >
> > ---
> >  v2:
> >   - changed __do_not_initialize to __no_initialize as requested by Kees
> >     Cook
> > ---
> >  drivers/android/binder.c | 4 ++--
> >  include/linux/wait.h     | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index a59871532ff6b..66984e7c33094 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -4827,7 +4827,7 @@ static int binder_ioctl_write_read(struct file *filp,
> >         struct binder_proc *proc = filp->private_data;
> >         unsigned int size = _IOC_SIZE(cmd);
> >         void __user *ubuf = (void __user *)arg;
> > -       struct binder_write_read bwr __no_initialize;
> > +       struct binder_write_read bwr;
>
> How did __no_initialize get set so that it can be removed here? Should
> the addition of __no_initilize be removed earlier in the series (tip
> doesn't have the __no_initialize).

Sorry, I messed up this patch. It should not be touching binder.c at
all. All binder changes should go into patch 2/3.


> >         case BINDER_SET_MAX_THREADS: {
> > -               int max_threads;
> > +               int max_threads __no_initialize;
>
> Is this really needed? A single integer in a rarely called ioctl()
> being initialized twice doesn't warrant this optimization.

It really does not, and I didn't have this bit in v1.
But if we don't want this diff to bit rot, we'd better have a
Coccinelle script generating it.
The script I added to the description of patch 2/3 introduced this
annotation, and I thought keeping it is better than trying to teach
the script about the size of the arguments.