Patchwork [1/2,v2] bitbake.conf: Add SECURITY_*FLAGS overridable definition

login
register
mail settings
Submitter Saul Wold
Date June 28, 2013, 7:23 p.m.
Message ID <1372447427-31750-1-git-send-email-sgw@linux.intel.com>
Download mbox | patch
Permalink /patch/52633/
State New
Headers show

Comments

Saul Wold - June 28, 2013, 7:23 p.m.
This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
defined in the security_flags.inc and override the empty default.

Signed-off-by: Saul Wold <sgw@linux.intel.com>
---
 meta/conf/bitbake.conf | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Mark Hatle - June 28, 2013, 7:28 p.m.
On 6/28/13 2:23 PM, Saul Wold wrote:
> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
> defined in the security_flags.inc and override the empty default.
>
> Signed-off-by: Saul Wold <sgw@linux.intel.com>
> ---
>   meta/conf/bitbake.conf | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> index 62a3936..72e1a78 100644
> --- a/meta/conf/bitbake.conf
> +++ b/meta/conf/bitbake.conf
> @@ -493,7 +493,8 @@ export TARGET_CPPFLAGS = ""
>
>   export BUILD_CFLAGS = "${BUILD_CPPFLAGS} ${BUILD_OPTIMIZATION}"
>   BUILDSDK_CFLAGS = "${BUILDSDK_CPPFLAGS} ${BUILD_OPTIMIZATION}"
> -export CFLAGS = "${TARGET_CFLAGS}"
> +SECURITY_CFLAGS ?= ""
> +export CFLAGS = "${TARGET_CFLAGS} ${SECURITY_CFLAGS}"
>   export TARGET_CFLAGS = "${TARGET_CPPFLAGS} ${SELECTED_OPTIMIZATION}"

I would prefer if the cflags came in via the TARGET_CFLAGS, similar to how the 
selected_optimization works.  In the past we've hacked around with CFLAGS itself 
and in the end it's always seem to have caused problems.  By modifing 
TARGET_CFLAGS instead, then the user can do (in a recipe):

CFLAGS = "-fmy-custom-flag ${TARGET_CFLAGS}"

>   export BUILD_CXXFLAGS = "${BUILD_CFLAGS} -fpermissive"
> @@ -523,7 +524,8 @@ LINKER_HASH_STYLE_mips64 = "sysv"
>   LINKER_HASH_STYLE_mips64el = "sysv"
>   TARGET_LINK_HASH_STYLE ?= "${@['-Wl,--hash-style=gnu',''][d.getVar('LINKER_HASH_STYLE', True) != 'gnu']}"
>
> -export LDFLAGS = "${TARGET_LDFLAGS}"
> +SECURITY_LDFLAGS ?= ""
> +export LDFLAGS = "${TARGET_LDFLAGS} ${SECURITY_LDFLAGS}"

Same comment here.

>   export TARGET_LDFLAGS = "-Wl,-O1 ${TARGET_LINK_HASH_STYLE}"
>   #export TARGET_LDFLAGS = "-L${STAGING_DIR_TARGET}${libdir} \
>   #                         -Wl,-rpath-link,${STAGING_DIR_TARGET}${libdir} \
>
Phil Blundell - June 28, 2013, 7:51 p.m.
On Fri, 2013-06-28 at 12:23 -0700, Saul Wold wrote:
> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
> defined in the security_flags.inc and override the empty default.

Why can't security_flags.inc just append to CFLAGS and LDFLAGS
respectively, or some other set of variables that already exists?

Creating new variables in bitbake.conf does have a cost in terms of
parse time and memory footprint for every recipe.  If the variables are
referenced in ${CFLAGS} etc then it also adds an extra substitution
whenever CFLAGS is expanded.  The cost of those things isn't enormous,
but it isn't zero either and adding them isn't something that we should
do capriciously.

p.
Saul Wold - June 28, 2013, 8:19 p.m.
On 06/28/2013 12:51 PM, Phil Blundell wrote:
> On Fri, 2013-06-28 at 12:23 -0700, Saul Wold wrote:
>> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
>> defined in the security_flags.inc and override the empty default.
>
> Why can't security_flags.inc just append to CFLAGS and LDFLAGS
> respectively, or some other set of variables that already exists?
>
So, if I remember correctly there was issues with this because there are 
a number of packages that have to modify specifically the security 
related flags (see the list in security_flags.inc), the ordering/timing 
of being able to due that correctly did not allow for setting it 
directly in CFLAGS or TARGET_CFLAGS.

> Creating new variables in bitbake.conf does have a cost in terms of
> parse time and memory footprint for every recipe.  If the variables are
> referenced in ${CFLAGS} etc then it also adds an extra substitution
> whenever CFLAGS is expanded.  The cost of those things isn't enormous,
> but it isn't zero either and adding them isn't something that we should
> do capriciously.
>
I understand, and RP and I talked about this, we needed a separate 
variable to ensure the correct substitution occurred for those that 
needed to disable or remove certain flags.

Sau!


> p.
>
>
>
>
Richard Purdie - June 28, 2013, 9:04 p.m.
On Fri, 2013-06-28 at 13:19 -0700, Saul Wold wrote:
> On 06/28/2013 12:51 PM, Phil Blundell wrote:
> > On Fri, 2013-06-28 at 12:23 -0700, Saul Wold wrote:
> >> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
> >> defined in the security_flags.inc and override the empty default.
> >
> > Why can't security_flags.inc just append to CFLAGS and LDFLAGS
> > respectively, or some other set of variables that already exists?
> >
> So, if I remember correctly there was issues with this because there are 
> a number of packages that have to modify specifically the security 
> related flags (see the list in security_flags.inc), the ordering/timing 
> of being able to due that correctly did not allow for setting it 
> directly in CFLAGS or TARGET_CFLAGS.
> 
> > Creating new variables in bitbake.conf does have a cost in terms of
> > parse time and memory footprint for every recipe.  If the variables are
> > referenced in ${CFLAGS} etc then it also adds an extra substitution
> > whenever CFLAGS is expanded.  The cost of those things isn't enormous,
> > but it isn't zero either and adding them isn't something that we should
> > do capriciously.
> >
> I understand, and RP and I talked about this, we needed a separate 
> variable to ensure the correct substitution occurred for those that 
> needed to disable or remove certain flags.

What RP said was that he'd prefer to see no bitbake.conf changes and to
do this all in the .inc. We should have a variable like the
SECURITY_FLAGS you have but this can also be appended in the .inc.

If we need to modify it on a per recipe basis we still then can so:

SECURITY_CFLAGS = "-fstack-protector-all -pie -fpie -D_FORTIFY_SOURCE=2"
TARGET_CFLAGS_append = " ${SECURITY_CFLAGS}"

SECURITY_LDFLAGS = "-Wl,-z,relro,-z,now"
TARGET_LDFLAGS_append = " ${SECURITY_LDFLAGS}"

all in the .inc. Or am I missing something?

Cheers,

Richard
Phil Blundell - June 28, 2013, 9:07 p.m.
On Fri, 2013-06-28 at 13:19 -0700, Saul Wold wrote:
> So, if I remember correctly there was issues with this because there are 
> a number of packages that have to modify specifically the security 
> related flags (see the list in security_flags.inc), the ordering/timing 
> of being able to due that correctly did not allow for setting it 
> directly in CFLAGS or TARGET_CFLAGS.

What exactly were the issues?  I can't think of any obvious reason why
it wouldn't work for security.inc to do:

SECURITY_CFLAGS = "-fstack-protector-all -pie -fpie -D_FORTIFY_SOURCE=2"
SECURITY_CFLAGS_pn-curl = "-fstack-protector-all -pie -fpie"
CFLAGS += "${SECURITY_CFLAGS}"

p.
Saul Wold - June 28, 2013, 9:52 p.m.
On 06/28/2013 02:07 PM, Phil Blundell wrote:
> On Fri, 2013-06-28 at 13:19 -0700, Saul Wold wrote:
>> So, if I remember correctly there was issues with this because there are
>> a number of packages that have to modify specifically the security
>> related flags (see the list in security_flags.inc), the ordering/timing
>> of being able to due that correctly did not allow for setting it
>> directly in CFLAGS or TARGET_CFLAGS.
>
> What exactly were the issues?  I can't think of any obvious reason why
> it wouldn't work for security.inc to do:
>
> SECURITY_CFLAGS = "-fstack-protector-all -pie -fpie -D_FORTIFY_SOURCE=2"
> SECURITY_CFLAGS_pn-curl = "-fstack-protector-all -pie -fpie"
> CFLAGS += "${SECURITY_CFLAGS}"
>
Seems either will work, this or TARGET_CFLAGS_append, I guess the 
problem I had in the past was trying to do it in the recipe or some 
other ordering problem.  I think part of it is that these are special 
since they use "export"

So now the question is which place them TARGET_*FLAGS or *FLAGS??

TARGET_* makes it clear we are modifying those flags.


Sau!
> p.
>
>
>
>
Khem Raj - June 28, 2013, 10:13 p.m.
On Jun 28, 2013, at 12:28 PM, Mark Hatle <mark.hatle@windriver.com> wrote:

> On 6/28/13 2:23 PM, Saul Wold wrote:
>> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
>> defined in the security_flags.inc and override the empty default.
>> 
>> Signed-off-by: Saul Wold <sgw@linux.intel.com>
>> ---
>>  meta/conf/bitbake.conf | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
>> index 62a3936..72e1a78 100644
>> --- a/meta/conf/bitbake.conf
>> +++ b/meta/conf/bitbake.conf
>> @@ -493,7 +493,8 @@ export TARGET_CPPFLAGS = ""
>> 
>>  export BUILD_CFLAGS = "${BUILD_CPPFLAGS} ${BUILD_OPTIMIZATION}"
>>  BUILDSDK_CFLAGS = "${BUILDSDK_CPPFLAGS} ${BUILD_OPTIMIZATION}"
>> -export CFLAGS = "${TARGET_CFLAGS}"
>> +SECURITY_CFLAGS ?= ""
>> +export CFLAGS = "${TARGET_CFLAGS} ${SECURITY_CFLAGS}"
>>  export TARGET_CFLAGS = "${TARGET_CPPFLAGS} ${SELECTED_OPTIMIZATION}"
> 
> I would prefer if the cflags came in via the TARGET_CFLAGS, similar to how the selected_optimization works.  In the past we've hacked around with CFLAGS itself and in the end it's always seem to have caused problems.  By modifing TARGET_CFLAGS instead, then the user can do (in a recipe):
> 
> CFLAGS = "-fmy-custom-flag ${TARGET_CFLAGS}"

yes infact it should be added via TARGET_CFLAGS 

> 
>>  export BUILD_CXXFLAGS = "${BUILD_CFLAGS} -fpermissive"
>> @@ -523,7 +524,8 @@ LINKER_HASH_STYLE_mips64 = "sysv"
>>  LINKER_HASH_STYLE_mips64el = "sysv"
>>  TARGET_LINK_HASH_STYLE ?= "${@['-Wl,--hash-style=gnu',''][d.getVar('LINKER_HASH_STYLE', True) != 'gnu']}"
>> 
>> -export LDFLAGS = "${TARGET_LDFLAGS}"
>> +SECURITY_LDFLAGS ?= ""
>> +export LDFLAGS = "${TARGET_LDFLAGS} ${SECURITY_LDFLAGS}"
> 
> Same comment here.
> 
>>  export TARGET_LDFLAGS = "-Wl,-O1 ${TARGET_LINK_HASH_STYLE}"
>>  #export TARGET_LDFLAGS = "-L${STAGING_DIR_TARGET}${libdir} \
>>  #                         -Wl,-rpath-link,${STAGING_DIR_TARGET}${libdir} \
>> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
Khem Raj - June 28, 2013, 10:16 p.m.
On Jun 28, 2013, at 12:51 PM, Phil Blundell <pb@pbcl.net> wrote:

> On Fri, 2013-06-28 at 12:23 -0700, Saul Wold wrote:
>> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
>> defined in the security_flags.inc and override the empty default.
> 
> Why can't security_flags.inc just append to CFLAGS and LDFLAGS
> respectively, or some other set of variables that already exists?
> 

It could help debugging metadata a bit if they are separate but appending to TARGET_FLAGS
in this inc file is not as bad an option

> Creating new variables in bitbake.conf does have a cost in terms of
> parse time and memory footprint for every recipe.  If the variables are
> referenced in ${CFLAGS} etc then it also adds an extra substitution
> whenever CFLAGS is expanded.  The cost of those things isn't enormous,
> but it isn't zero either and adding them isn't something that we should
> do capriciously.
> 
> p.
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
Khem Raj - June 28, 2013, 10:17 p.m.
On Jun 28, 2013, at 1:19 PM, Saul Wold <sgw@linux.intel.com> wrote:

> On 06/28/2013 12:51 PM, Phil Blundell wrote:
>> On Fri, 2013-06-28 at 12:23 -0700, Saul Wold wrote:
>>> This will allow for SECURITY_CFLAGS and SECURITY_LDFLAGS to be
>>> defined in the security_flags.inc and override the empty default.
>> 
>> Why can't security_flags.inc just append to CFLAGS and LDFLAGS
>> respectively, or some other set of variables that already exists?
>> 
> So, if I remember correctly there was issues with this because there are a number of packages that have to modify specifically the security related flags (see the list in security_flags.inc), the ordering/timing of being able to due that correctly did not allow for setting it directly in CFLAGS or TARGET_CFLAGS.

can you use append|prepend override ?

> 
>> Creating new variables in bitbake.conf does have a cost in terms of
>> parse time and memory footprint for every recipe.  If the variables are
>> referenced in ${CFLAGS} etc then it also adds an extra substitution
>> whenever CFLAGS is expanded.  The cost of those things isn't enormous,
>> but it isn't zero either and adding them isn't something that we should
>> do capriciously.
>> 
> I understand, and RP and I talked about this, we needed a separate variable to ensure the correct substitution occurred for those that needed to disable or remove certain flags.
> 
> Sau!
> 
> 
>> p.
>> 
>> 
>> 
>> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Patch

diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 62a3936..72e1a78 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -493,7 +493,8 @@  export TARGET_CPPFLAGS = ""
 
 export BUILD_CFLAGS = "${BUILD_CPPFLAGS} ${BUILD_OPTIMIZATION}"
 BUILDSDK_CFLAGS = "${BUILDSDK_CPPFLAGS} ${BUILD_OPTIMIZATION}"
-export CFLAGS = "${TARGET_CFLAGS}"
+SECURITY_CFLAGS ?= ""
+export CFLAGS = "${TARGET_CFLAGS} ${SECURITY_CFLAGS}"
 export TARGET_CFLAGS = "${TARGET_CPPFLAGS} ${SELECTED_OPTIMIZATION}"
 
 export BUILD_CXXFLAGS = "${BUILD_CFLAGS} -fpermissive"
@@ -523,7 +524,8 @@  LINKER_HASH_STYLE_mips64 = "sysv"
 LINKER_HASH_STYLE_mips64el = "sysv"
 TARGET_LINK_HASH_STYLE ?= "${@['-Wl,--hash-style=gnu',''][d.getVar('LINKER_HASH_STYLE', True) != 'gnu']}"
 
-export LDFLAGS = "${TARGET_LDFLAGS}"
+SECURITY_LDFLAGS ?= ""
+export LDFLAGS = "${TARGET_LDFLAGS} ${SECURITY_LDFLAGS}"
 export TARGET_LDFLAGS = "-Wl,-O1 ${TARGET_LINK_HASH_STYLE}"
 #export TARGET_LDFLAGS = "-L${STAGING_DIR_TARGET}${libdir} \
 #                         -Wl,-rpath-link,${STAGING_DIR_TARGET}${libdir} \