Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1602932 > unrolled thread

[PATCH 6/7] md/raid10, LLVM: get rid of variable length array

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Michael Davidson <md@google.com> - 2017-03-17 01:30 +0100
    Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-17 13:20 +0100
      Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Alexander Potapenko <glider@google.com> - 2017-03-17 13:40 +0100
        Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-17 13:50 +0100
          Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Michael Davidson <md@google.com> - 2017-03-17 20:30 +0100
            Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-17 21:10 +0100
              Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array hpa@zytor.com - 2017-03-17 21:50 +0100
                Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Dmitry Vyukov <dvyukov@google.com> - 2017-03-24 14:50 +0100
                  Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-24 15:20 +0100
        Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Alexander Potapenko <glider@google.com> - 2017-03-17 13:50 +0100
          Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Borislav Petkov <bp@alien8.de> - 2017-03-17 19:10 +0100
            Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Dmitry Vyukov <dvyukov@google.com> - 2017-03-17 19:50 +0100
              Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Borislav Petkov <bp@alien8.de> - 2017-03-17 20:10 +0100
                Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Dmitry Vyukov <dvyukov@google.com> - 2017-03-17 20:10 +0100
                  Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-17 20:40 +0100
                    Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Dmitry Vyukov <dvyukov@google.com> - 2017-03-24 15:00 +0100
                      Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-24 15:20 +0100
                        Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Dmitry Vyukov <dvyukov@google.com> - 2017-03-24 15:30 +0100
                  Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Peter Zijlstra <peterz@infradead.org> - 2017-03-17 21:40 +0100
                  Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array Fengguang Wu <fengguang.wu@intel.com> - 2017-03-18 01:50 +0100

#1602932 — [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

Replace a variable length array in a struct by allocating
the memory for the entire struct in a char array on the stack.

Signed-off-by: Michael Davidson <md@google.com>
---
 drivers/md/raid10.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 063c43d83b72..158ebdff782c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
 	/* Use sync reads to get the blocks from somewhere else */
 	int sectors = r10_bio->sectors;
 	struct r10conf *conf = mddev->private;
-	struct {
-		struct r10bio r10_bio;
-		struct r10dev devs[conf->copies];
-	} on_stack;
-	struct r10bio *r10b = &on_stack.r10_bio;
+	char on_stack_r10_bio[sizeof(struct r10bio) +
+			      conf->copies * sizeof(struct r10dev)]
+			      __aligned(__alignof__(struct r10bio));
+	struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
 	int slot = 0;
 	int idx = 0;
 	struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
-- 
2.12.0.367.g23dc2f6d3c-goog

[toc] | [next] | [standalone]

#1603252

On Thu, Mar 16, 2017 at 05:15:19PM -0700, Michael Davidson wrote:
> Replace a variable length array in a struct by allocating
> the memory for the entire struct in a char array on the stack.
> 
> Signed-off-by: Michael Davidson <md@google.com>
> ---
>  drivers/md/raid10.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 063c43d83b72..158ebdff782c 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
>  	/* Use sync reads to get the blocks from somewhere else */
>  	int sectors = r10_bio->sectors;
>  	struct r10conf *conf = mddev->private;
> -	struct {
> -		struct r10bio r10_bio;
> -		struct r10dev devs[conf->copies];
> -	} on_stack;
> -	struct r10bio *r10b = &on_stack.r10_bio;
> +	char on_stack_r10_bio[sizeof(struct r10bio) +
> +			      conf->copies * sizeof(struct r10dev)]
> +			      __aligned(__alignof__(struct r10bio));
> +	struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
>  	int slot = 0;
>  	int idx = 0;
>  	struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;


That's disgusting. Why not fix LLVM to support this?

[toc] | [prev] | [next] | [standalone]

#1603275

On Fri, Mar 17, 2017 at 1:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Mar 16, 2017 at 05:15:19PM -0700, Michael Davidson wrote:
>> Replace a variable length array in a struct by allocating
>> the memory for the entire struct in a char array on the stack.
>>
>> Signed-off-by: Michael Davidson <md@google.com>
>> ---
>>  drivers/md/raid10.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 063c43d83b72..158ebdff782c 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
>>       /* Use sync reads to get the blocks from somewhere else */
>>       int sectors = r10_bio->sectors;
>>       struct r10conf *conf = mddev->private;
>> -     struct {
>> -             struct r10bio r10_bio;
>> -             struct r10dev devs[conf->copies];
>> -     } on_stack;
>> -     struct r10bio *r10b = &on_stack.r10_bio;
>> +     char on_stack_r10_bio[sizeof(struct r10bio) +
>> +                           conf->copies * sizeof(struct r10dev)]
>> +                           __aligned(__alignof__(struct r10bio));
>> +     struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
>>       int slot = 0;
>>       int idx = 0;
>>       struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
>
>
> That's disgusting. Why not fix LLVM to support this?

IIUC there's only a handful of VLAIS instances in LLVM code, why not
just drop them for the sake of better code portability?
(To quote Linus, "this feature is an abomination":
https://lkml.org/lkml/2013/9/23/500)

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [next] | [standalone]

#1603279

On Fri, Mar 17, 2017 at 01:31:23PM +0100, Alexander Potapenko wrote:
> On Fri, Mar 17, 2017 at 1:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Mar 16, 2017 at 05:15:19PM -0700, Michael Davidson wrote:
> >> Replace a variable length array in a struct by allocating
> >> the memory for the entire struct in a char array on the stack.
> >>
> >> Signed-off-by: Michael Davidson <md@google.com>
> >> ---
> >>  drivers/md/raid10.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >> index 063c43d83b72..158ebdff782c 100644
> >> --- a/drivers/md/raid10.c
> >> +++ b/drivers/md/raid10.c
> >> @@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
> >>       /* Use sync reads to get the blocks from somewhere else */
> >>       int sectors = r10_bio->sectors;
> >>       struct r10conf *conf = mddev->private;
> >> -     struct {
> >> -             struct r10bio r10_bio;
> >> -             struct r10dev devs[conf->copies];
> >> -     } on_stack;
> >> -     struct r10bio *r10b = &on_stack.r10_bio;
> >> +     char on_stack_r10_bio[sizeof(struct r10bio) +
> >> +                           conf->copies * sizeof(struct r10dev)]
> >> +                           __aligned(__alignof__(struct r10bio));
> >> +     struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
> >>       int slot = 0;
> >>       int idx = 0;
> >>       struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
> >
> >
> > That's disgusting. Why not fix LLVM to support this?
> 
> IIUC there's only a handful of VLAIS instances in LLVM code, why not
> just drop them for the sake of better code portability?
> (To quote Linus, "this feature is an abomination":
> https://lkml.org/lkml/2013/9/23/500)

Be that as it may; what you construct above is disgusting. Surely the
code can be refactored to not look like dog vomit?

Also; its not immediately obvious conf->copies is 'small' and this
doesn't blow up the stack; I feel that deserves a comment somewhere.

[toc] | [prev] | [next] | [standalone]

#1603588

On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Be that as it may; what you construct above is disgusting. Surely the
> code can be refactored to not look like dog vomit?
>
> Also; its not immediately obvious conf->copies is 'small' and this
> doesn't blow up the stack; I feel that deserves a comment somewhere.
>

I agree that the code is horrible.

It is, in fact, exactly the same solution that was used to remove
variable length arrays in structs from several of the crypto drivers a
few years ago - see the definition of SHASH_DESC_ON_STACK() in
"crypto/hash.h" - I did not, however, hide the horrors in a macro
preferring to leave the implementation visible as a warning to whoever
might touch the code next.

I believe that the actual stack usage is exactly the same as it was previously.

I can certainly wrap this  up in a macro and add comments with
appropriately dire warnings in it if you feel that is both necessary
and sufficient.

[toc] | [prev] | [next] | [standalone]

#1603599

On Fri, Mar 17, 2017 at 11:52:01AM -0700, Michael Davidson wrote:
> On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Be that as it may; what you construct above is disgusting. Surely the
> > code can be refactored to not look like dog vomit?
> >
> > Also; its not immediately obvious conf->copies is 'small' and this
> > doesn't blow up the stack; I feel that deserves a comment somewhere.
> >
> 
> I agree that the code is horrible.
> 
> It is, in fact, exactly the same solution that was used to remove
> variable length arrays in structs from several of the crypto drivers a
> few years ago - see the definition of SHASH_DESC_ON_STACK() in
> "crypto/hash.h" - I did not, however, hide the horrors in a macro
> preferring to leave the implementation visible as a warning to whoever
> might touch the code next.
> 
> I believe that the actual stack usage is exactly the same as it was previously.
> 
> I can certainly wrap this  up in a macro and add comments with
> appropriately dire warnings in it if you feel that is both necessary
> and sufficient.

We got away with ugly in the past, so we should get to do it again?

[toc] | [prev] | [next] | [standalone]

#1603617

On March 17, 2017 12:27:46 PM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Fri, Mar 17, 2017 at 11:52:01AM -0700, Michael Davidson wrote:
>> On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra
><peterz@infradead.org> wrote:
>> >
>> > Be that as it may; what you construct above is disgusting. Surely
>the
>> > code can be refactored to not look like dog vomit?
>> >
>> > Also; its not immediately obvious conf->copies is 'small' and this
>> > doesn't blow up the stack; I feel that deserves a comment
>somewhere.
>> >
>> 
>> I agree that the code is horrible.
>> 
>> It is, in fact, exactly the same solution that was used to remove
>> variable length arrays in structs from several of the crypto drivers
>a
>> few years ago - see the definition of SHASH_DESC_ON_STACK() in
>> "crypto/hash.h" - I did not, however, hide the horrors in a macro
>> preferring to leave the implementation visible as a warning to
>whoever
>> might touch the code next.
>> 
>> I believe that the actual stack usage is exactly the same as it was
>previously.
>> 
>> I can certainly wrap this  up in a macro and add comments with
>> appropriately dire warnings in it if you feel that is both necessary
>> and sufficient.
>
>We got away with ugly in the past, so we should get to do it again?

Seriously, you should have taken the hack the first time that this needs to be fixed.  Just because this is a fairly uncommon construct in the kernel doesn't mean it is not in userspace.

I would like to say this falls in the category of "fix your compiler this time".  Once is one thing, twice is unacceptable.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1608399

On Fri, Mar 17, 2017 at 9:04 PM,  <hpa@zytor.com> wrote:
> On March 17, 2017 12:27:46 PM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>>On Fri, Mar 17, 2017 at 11:52:01AM -0700, Michael Davidson wrote:
>>> On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra
>><peterz@infradead.org> wrote:
>>> >
>>> > Be that as it may; what you construct above is disgusting. Surely
>>the
>>> > code can be refactored to not look like dog vomit?
>>> >
>>> > Also; its not immediately obvious conf->copies is 'small' and this
>>> > doesn't blow up the stack; I feel that deserves a comment
>>somewhere.
>>> >
>>>
>>> I agree that the code is horrible.
>>>
>>> It is, in fact, exactly the same solution that was used to remove
>>> variable length arrays in structs from several of the crypto drivers
>>a
>>> few years ago - see the definition of SHASH_DESC_ON_STACK() in
>>> "crypto/hash.h" - I did not, however, hide the horrors in a macro
>>> preferring to leave the implementation visible as a warning to
>>whoever
>>> might touch the code next.
>>>
>>> I believe that the actual stack usage is exactly the same as it was
>>previously.
>>>
>>> I can certainly wrap this  up in a macro and add comments with
>>> appropriately dire warnings in it if you feel that is both necessary
>>> and sufficient.
>>
>>We got away with ugly in the past, so we should get to do it again?
>
> Seriously, you should have taken the hack the first time that this needs to be fixed.  Just because this is a fairly uncommon construct in the kernel doesn't mean it is not in userspace.


There is a reason why it is fairly uncommon in kernel.
Initially it was used more widely, but then there was a decision to
drop all uses of this feature. Namely:
Linus: "We should definitely drop it. The feature is an abomination".
https://lkml.org/lkml/2013/9/23/500
I really don't understand why you cling onto this last use of the
feature. Having a single use of a compiler extension on an error path
of a non-mandatory driver does not look like a great idea to me. Let's
just kill it off outside of clang discussion.

[toc] | [prev] | [next] | [standalone]

#1608453

On Fri, Mar 24, 2017 at 02:47:15PM +0100, Dmitry Vyukov wrote:
> > Seriously, you should have taken the hack the first time that this
> > needs to be fixed.  Just because this is a fairly uncommon construct
> > in the kernel doesn't mean it is not in userspace.
> 
> There is a reason why it is fairly uncommon in kernel.

So first off; its not entirely clear that the code as it exists it
correct. From a cursory reading of it and surrounding code, there is no
actual upper limit on the variable. If I were stupid enough to make a
raid with 64 devices I'd get a huge on-stack structure.

Since you're touching it; you should check these things.

And secondly, refactor the code to not look like dog vomit. You can do
more than the absolute minimal patch to make it compile, I'm sure.

[toc] | [prev] | [next] | [standalone]

#1603285

On Fri, Mar 17, 2017 at 1:31 PM, Alexander Potapenko <glider@google.com> wrote:
> On Fri, Mar 17, 2017 at 1:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Thu, Mar 16, 2017 at 05:15:19PM -0700, Michael Davidson wrote:
>>> Replace a variable length array in a struct by allocating
>>> the memory for the entire struct in a char array on the stack.
>>>
>>> Signed-off-by: Michael Davidson <md@google.com>
>>> ---
>>>  drivers/md/raid10.c | 9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>> index 063c43d83b72..158ebdff782c 100644
>>> --- a/drivers/md/raid10.c
>>> +++ b/drivers/md/raid10.c
>>> @@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
>>>       /* Use sync reads to get the blocks from somewhere else */
>>>       int sectors = r10_bio->sectors;
>>>       struct r10conf *conf = mddev->private;
>>> -     struct {
>>> -             struct r10bio r10_bio;
>>> -             struct r10dev devs[conf->copies];
>>> -     } on_stack;
>>> -     struct r10bio *r10b = &on_stack.r10_bio;
>>> +     char on_stack_r10_bio[sizeof(struct r10bio) +
>>> +                           conf->copies * sizeof(struct r10dev)]
>>> +                           __aligned(__alignof__(struct r10bio));
>>> +     struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;
>>>       int slot = 0;
>>>       int idx = 0;
>>>       struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
>>
>>
>> That's disgusting. Why not fix LLVM to support this?
>
> IIUC there's only a handful of VLAIS instances in LLVM code, why not
Sorry, "kernel code", not "LLVM code".
> just drop them for the sake of better code portability?
> (To quote Linus, "this feature is an abomination":
> https://lkml.org/lkml/2013/9/23/500)
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [next] | [standalone]

#1603519

On Fri, Mar 17, 2017 at 01:32:00PM +0100, Alexander Potapenko wrote:
> > IIUC there's only a handful of VLAIS instances in LLVM code, why not
> Sorry, "kernel code", not "LLVM code".
> > just drop them for the sake of better code portability?

And what happens if someone else adds a variable thing like this
somewhere else, builds with gcc, everything's fine and patch gets
applied? Or something else llvm can't stomach.

Does that mean there'll be the occasional, every-so-often whack-a-mole
patchset from someone, fixing the kernel build with llvm yet again?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1603548

On Fri, Mar 17, 2017 at 7:03 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Mar 17, 2017 at 01:32:00PM +0100, Alexander Potapenko wrote:
>> > IIUC there's only a handful of VLAIS instances in LLVM code, why not
>> Sorry, "kernel code", not "LLVM code".
>> > just drop them for the sake of better code portability?
>
> And what happens if someone else adds a variable thing like this
> somewhere else, builds with gcc, everything's fine and patch gets
> applied? Or something else llvm can't stomach.
>
> Does that mean there'll be the occasional, every-so-often whack-a-mole
> patchset from someone, fixing the kernel build with llvm yet again?


This problem is more general and is not specific to clang. It equally
applies to different versions of gcc, different arches and different
configs (namely, anything else than what a developer used for
testing). A known, reasonably well working solution to this problem is
a system of try bots that test patches before commit with different
compilers/configs/archs. We already have such system in the form of
0-day bots. It would be useful to extend it with clang as soon as
kernel builds.

[toc] | [prev] | [next] | [standalone]

#1603583

On Fri, Mar 17, 2017 at 07:47:33PM +0100, Dmitry Vyukov wrote:
> This problem is more general and is not specific to clang. It equally
> applies to different versions of gcc, different arches and different
> configs (namely, anything else than what a developer used for
> testing).

I guess. We do carry a bunch of gcc workarounds along with the cc-*
macros in scripts/Kbuild.include.

> A known, reasonably well working solution to this problem is
> a system of try bots that test patches before commit with different
> compilers/configs/archs. We already have such system in the form of
> 0-day bots. It would be useful to extend it with clang as soon as
> kernel builds.

Has someone actually already talked to Fengguang about it?

Oh, and the stupid question: why the effort to build the kernel
with clang at all? Just because or are there some actual, palpable
advantages?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1603584

On Fri, Mar 17, 2017 at 7:57 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Mar 17, 2017 at 07:47:33PM +0100, Dmitry Vyukov wrote:
>> This problem is more general and is not specific to clang. It equally
>> applies to different versions of gcc, different arches and different
>> configs (namely, anything else than what a developer used for
>> testing).
>
> I guess. We do carry a bunch of gcc workarounds along with the cc-*
> macros in scripts/Kbuild.include.
>
>> A known, reasonably well working solution to this problem is
>> a system of try bots that test patches before commit with different
>> compilers/configs/archs. We already have such system in the form of
>> 0-day bots. It would be useful to extend it with clang as soon as
>> kernel builds.
>
> Has someone actually already talked to Fengguang about it?

+Fengguang

> Oh, and the stupid question: why the effort to build the kernel
> with clang at all? Just because or are there some actual, palpable
> advantages?

On our side it is:
 - clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
 - better code coverage for fuzzing
 - why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)

You can also find some reasons in the Why section of LLVM-Linux project:
http://llvm.linuxfoundation.org/index.php/Main_Page

[toc] | [prev] | [next] | [standalone]

#1603590

On Fri, Mar 17, 2017 at 08:26:42PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
> > You can also find some reasons in the Why section of LLVM-Linux project:
> > http://llvm.linuxfoundation.org/index.php/Main_Page
> 
> From that:
> 
>  - LLVM/Clang is a fast moving project with many things fixed quickly
>    and features added.
> 
> So what's the deal with that 5 year old bug you want us to work around?
> 
> Also, clang doesn't support asm cc flags output and a few other
> extensions last time I checked.
> 

Another great one:

 - BSD License (some people prefer this license to the GPL)

Seems a very weak argument to make when talking about the Linux Kernel
which is very explicitly GPLv2 (and not later).

[toc] | [prev] | [next] | [standalone]

#1608412

On Fri, Mar 17, 2017 at 8:29 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Mar 17, 2017 at 08:26:42PM +0100, Peter Zijlstra wrote:
>> On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
>> > You can also find some reasons in the Why section of LLVM-Linux project:
>> > http://llvm.linuxfoundation.org/index.php/Main_Page
>>
>> From that:
>>
>>  - LLVM/Clang is a fast moving project with many things fixed quickly
>>    and features added.
>>
>> So what's the deal with that 5 year old bug you want us to work around?
>>
>> Also, clang doesn't support asm cc flags output and a few other
>> extensions last time I checked.
>>
>
> Another great one:
>
>  - BSD License (some people prefer this license to the GPL)
>
> Seems a very weak argument to make when talking about the Linux Kernel
> which is very explicitly GPLv2 (and not later).

OK, I guess should not have referenced the llvm-linux page.
So here are reasons on our side that I am ready to vouch:

 - clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
 - better code coverage for fuzzing
 - why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)

Michael is on a different team and has own reasons to do this.

[toc] | [prev] | [next] | [standalone]

#1608459

On Fri, Mar 24, 2017 at 02:50:24PM +0100, Dmitry Vyukov wrote:
> OK, I guess should not have referenced the llvm-linux page.
> So here are reasons on our side that I am ready to vouch:
> 
>  - clang make it possible to implement KMSAN (dynamic detection of
> uses of uninit memory)

How does GCC make this impossible?

>  - better code coverage for fuzzing

How so? Why can't the same be achieved using GCC?

>  - why simpler and faster development (e.g. we can port our user-space
> hardening technologies -- CFI and SafeStack)

That's just because you've already implemented this in clang, right? So
less work for you. Not because its impossible.

[toc] | [prev] | [next] | [standalone]

#1608474

On Fri, Mar 24, 2017 at 3:10 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Mar 24, 2017 at 02:50:24PM +0100, Dmitry Vyukov wrote:
>> OK, I guess should not have referenced the llvm-linux page.
>> So here are reasons on our side that I am ready to vouch:
>>
>>  - clang make it possible to implement KMSAN (dynamic detection of
>> uses of uninit memory)
>
> How does GCC make this impossible?

Too complex and too difficult to implement correctly on all corner
cases. All other sanitizers were ported to gcc very quickly, but msan
wasn't. Nobody is brave enough to even approach it.

>>  - better code coverage for fuzzing
>
> How so? Why can't the same be achieved using GCC?

Same reason.

>>  - why simpler and faster development (e.g. we can port our user-space
>> hardening technologies -- CFI and SafeStack)
>
> That's just because you've already implemented this in clang, right? So
> less work for you. Not because its impossible.

I am not saying that it's impossible. It would just require
unreasonable amount of time, and then perpetual maintenance to fix
corner cases and regressions.

For background: I implemented the current fuzzing coverage (KCOV) in
gcc, and user-space tsan instrumentation in gcc.

[toc] | [prev] | [next] | [standalone]

#1603611

On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
> You can also find some reasons in the Why section of LLVM-Linux project:
> http://llvm.linuxfoundation.org/index.php/Main_Page

From that:

 - LLVM/Clang is a fast moving project with many things fixed quickly
   and features added.

So what's the deal with that 5 year old bug you want us to work around?

Also, clang doesn't support asm cc flags output and a few other
extensions last time I checked.

[toc] | [prev] | [next] | [standalone]

#1603681

Hi Dmitry,
 
On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
>On Fri, Mar 17, 2017 at 7:57 PM, Borislav Petkov <bp@alien8.de> wrote:
>> On Fri, Mar 17, 2017 at 07:47:33PM +0100, Dmitry Vyukov wrote:
>>> This problem is more general and is not specific to clang. It equally
>>> applies to different versions of gcc, different arches and different
>>> configs (namely, anything else than what a developer used for
>>> testing).
>>
>> I guess. We do carry a bunch of gcc workarounds along with the cc-*
>> macros in scripts/Kbuild.include.
>>
>>> A known, reasonably well working solution to this problem is
>>> a system of try bots that test patches before commit with different
>>> compilers/configs/archs. We already have such system in the form of
>>> 0-day bots. It would be useful to extend it with clang as soon as
>>> kernel builds.
>>
>> Has someone actually already talked to Fengguang about it?
>
>+Fengguang

I've actually tried clang long time ago. It quickly fails the build
for vanilla kernel. So it really depends on when the various clang
build fix patches can be accepted into mainline kernel.

Thanks,
Fengguang

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web