Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200302130430.201037-1-glider@google.com> <20200302130430.201037-3-glider@google.com>
 <CAHRSSEwe=jZAEVhGw4ACBU0m-76TzZfJFv1Rzw=_UVm6HbTvAw@mail.gmail.com>
In-Reply-To: <CAHRSSEwe=jZAEVhGw4ACBU0m-76TzZfJFv1Rzw=_UVm6HbTvAw@mail.gmail.com>
From:   Alexander Potapenko <glider@google.com>
Date:   Mon, 2 Mar 2020 19:03:19 +0100
Message-ID: <CAG_fn=W96H3kMcoTxfqVq9Ec=1HZsJjnTjuX74dhZJe0QNuMrw@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event()
To:     Todd Kjos <tkjos@google.com>
Cc:     Kees Cook <keescook@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        Ingo Molnar <mingo@redhat.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Jann Horn <jannh@google.com>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.