From: Alexander Potapenko <glider@google.com> To: Todd Kjos <tkjos@google.com> Cc: "Kees Cook" <keescook@chromium.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Arve Hjønnevåg" <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> Subject: Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() Date: Mon, 2 Mar 2020 19:03:19 +0100 [thread overview] Message-ID: <CAG_fn=W96H3kMcoTxfqVq9Ec=1HZsJjnTjuX74dhZJe0QNuMrw@mail.gmail.com> (raw) In-Reply-To: <CAHRSSEwe=jZAEVhGw4ACBU0m-76TzZfJFv1Rzw=_UVm6HbTvAw@mail.gmail.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.
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Potapenko <glider@google.com> To: Todd Kjos <tkjos@google.com> Cc: "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>, "Kees Cook" <keescook@chromium.org>, "Jann Horn" <jannh@google.com>, "Peter Zijlstra" <peterz@infradead.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, LKML <linux-kernel@vger.kernel.org>, "Arve Hjønnevåg" <arve@android.com>, "Ingo Molnar" <mingo@redhat.com>, "Dmitry Vyukov" <dvyukov@google.com> Subject: Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() Date: Mon, 2 Mar 2020 19:03:19 +0100 [thread overview] Message-ID: <CAG_fn=W96H3kMcoTxfqVq9Ec=1HZsJjnTjuX74dhZJe0QNuMrw@mail.gmail.com> (raw) In-Reply-To: <CAHRSSEwe=jZAEVhGw4ACBU0m-76TzZfJFv1Rzw=_UVm6HbTvAw@mail.gmail.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. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-03-02 18:03 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-02 13:04 [PATCH v2 1/3] compiler.h: define __no_initialize glider 2020-03-02 13:04 ` glider 2020-03-02 13:04 ` [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() glider 2020-03-02 13:04 ` glider 2020-03-02 13:09 ` Joe Perches 2020-03-02 13:09 ` Joe Perches 2020-03-02 13:25 ` Alexander Potapenko 2020-03-02 13:25 ` Alexander Potapenko 2020-03-02 13:52 ` Dan Carpenter 2020-03-02 13:52 ` Dan Carpenter 2020-03-02 13:58 ` Joe Perches 2020-03-02 13:58 ` Joe Perches 2020-03-02 18:17 ` Alexander Potapenko 2020-03-02 18:17 ` Alexander Potapenko 2020-03-02 18:31 ` Jann Horn 2020-03-02 18:31 ` Jann Horn 2020-03-05 9:03 ` Rasmus Villemoes 2020-03-05 9:03 ` Rasmus Villemoes 2020-03-05 12:45 ` Jann Horn 2020-03-05 12:45 ` Jann Horn 2020-03-06 2:29 ` Al Viro 2020-03-06 2:29 ` Al Viro 2020-03-02 18:50 ` Joe Perches 2020-03-02 18:50 ` Joe Perches 2020-03-03 9:14 ` Alexander Potapenko 2020-03-03 9:14 ` Alexander Potapenko 2020-03-03 9:38 ` Dan Carpenter 2020-03-03 9:38 ` Dan Carpenter 2020-03-03 13:56 ` Joe Perches 2020-03-03 13:56 ` Joe Perches 2020-03-03 14:15 ` Dan Carpenter 2020-03-03 14:15 ` Dan Carpenter 2020-03-04 18:13 ` Kees Cook 2020-03-04 18:13 ` Kees Cook 2020-03-05 8:07 ` Dan Carpenter 2020-03-05 8:07 ` Dan Carpenter 2020-03-05 8:26 ` Kees Cook 2020-03-05 8:26 ` Kees Cook 2020-03-05 8:33 ` Alexander Potapenko 2020-03-05 8:33 ` Alexander Potapenko 2020-03-02 17:38 ` Greg KH 2020-03-02 17:38 ` Greg KH 2020-03-02 18:28 ` Alexander Potapenko 2020-03-02 18:28 ` Alexander Potapenko 2020-03-02 13:04 ` [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() glider 2020-03-02 13:04 ` glider 2020-03-02 16:56 ` Todd Kjos 2020-03-02 16:56 ` Todd Kjos 2020-03-02 18:03 ` Alexander Potapenko [this message] 2020-03-02 18:03 ` Alexander Potapenko 2020-03-02 18:39 ` Greg Kroah-Hartman 2020-03-02 18:39 ` Greg Kroah-Hartman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAG_fn=W96H3kMcoTxfqVq9Ec=1HZsJjnTjuX74dhZJe0QNuMrw@mail.gmail.com' \ --to=glider@google.com \ --cc=arve@android.com \ --cc=devel@driverdev.osuosl.org \ --cc=dvyukov@google.com \ --cc=gregkh@linuxfoundation.org \ --cc=jannh@google.com \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=tkjos@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.