[PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Alex Kiernan]

Since UBOOT_DTB_BINARY empty means we don't need to inject signatures
into the U-Boot DTB, we can remove the dependencies between consumers of
these two classes and resolve a circular dependency between u-boot and
kernel.

Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
---

 meta/classes/kernel-fitimage.bbclass | 2 +-
 meta/classes/uboot-sign.bbclass      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
index 2820ff968940..9e224daf057e 100644
--- a/meta/classes/kernel-fitimage.bbclass
+++ b/meta/classes/kernel-fitimage.bbclass
@@ -42,7 +42,7 @@ python __anonymous () {
         # Verified boot will sign the fitImage and append the public key to
         # U-Boot dtb. We ensure the U-Boot dtb is deployed before assembling
         # the fitImage:
-        if d.getVar('UBOOT_SIGN_ENABLE') == "1":
+        if d.getVar('UBOOT_SIGN_ENABLE') == "1" and d.getVar('UBOOT_DTB_BINARY'):
             uboot_pn = d.getVar('PREFERRED_PROVIDER_u-boot') or 'u-boot'
             d.appendVarFlag('do_assemble_fitimage', 'depends', ' %s:do_populate_sysroot' % uboot_pn)
 }
diff --git a/meta/classes/uboot-sign.bbclass b/meta/classes/uboot-sign.bbclass
index 8beafff7c03e..de81ad1b389c 100644
--- a/meta/classes/uboot-sign.bbclass
+++ b/meta/classes/uboot-sign.bbclass
@@ -116,7 +116,7 @@ do_install_append() {
 }
 
 python () {
-    if d.getVar('UBOOT_SIGN_ENABLE') == '1' and d.getVar('PN') == d.getVar('UBOOT_PN'):
+    if d.getVar('UBOOT_SIGN_ENABLE') == '1' and d.getVar('PN') == d.getVar('UBOOT_PN') and d.getVar('UBOOT_DTB_BINARY'):
         kernel_pn = d.getVar('PREFERRED_PROVIDER_virtual/kernel')
 
         # Make "bitbake u-boot -cdeploy" deploys the signed u-boot.dtb
-- 
2.17.1

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Richard Purdie]

On Wed, 2019-06-12 at 09:15 +0100, Alex Kiernan wrote:
> Since UBOOT_DTB_BINARY empty means we don't need to inject signatures
> into the U-Boot DTB, we can remove the dependencies between consumers
> of
> these two classes and resolve a circular dependency between u-boot
> and
> kernel.
> 
> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> ---
> 
>  meta/classes/kernel-fitimage.bbclass | 2 +-
>  meta/classes/uboot-sign.bbclass      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Really wish we had tests and documentation for these classes...

Cheers,

Richard

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Alex Kiernan]

On Wed, Jun 12, 2019 at 10:59 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Wed, 2019-06-12 at 09:15 +0100, Alex Kiernan wrote:
> > Since UBOOT_DTB_BINARY empty means we don't need to inject signatures
> > into the U-Boot DTB, we can remove the dependencies between consumers
> > of
> > these two classes and resolve a circular dependency between u-boot
> > and
> > kernel.
> >
> > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> > ---
> >
> >  meta/classes/kernel-fitimage.bbclass | 2 +-
> >  meta/classes/uboot-sign.bbclass      | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
>
> Really wish we had tests and documentation for these classes...
>

I know... I have a dread anytime anyone touches it, me included... I
sat on this change for a couple of weeks too.

The truth is the pair of them are fragile and hopelessly intertwined,
but without tests they're never going to get rewritten, so I might try
and take some of that on (or get someone here to do it) as I still
have multiple issues with them - signing doesn't really work the way I
want, I still have a circular dependency I've not tracked down,
doubtless other things I can't immediately bring to mind.

If we were to add some tests around it, what would you pull out as the
simplest thing that it could do which would be valuable?

-- 
Alex Kiernan

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Michael Scott]

Hi Richard,

On 6/12/19 11:02 AM, Alex Kiernan wrote:
> On Wed, Jun 12, 2019 at 10:59 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>> On Wed, 2019-06-12 at 09:15 +0100, Alex Kiernan wrote:
>>> Since UBOOT_DTB_BINARY empty means we don't need to inject signatures
>>> into the U-Boot DTB, we can remove the dependencies between consumers
>>> of
>>> these two classes and resolve a circular dependency between u-boot
>>> and
>>> kernel.
>>>
>>> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
>>> ---
>>>
>>>   meta/classes/kernel-fitimage.bbclass | 2 +-
>>>   meta/classes/uboot-sign.bbclass      | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>> Really wish we had tests and documentation for these classes...
>>
> I know... I have a dread anytime anyone touches it, me included... I
> sat on this change for a couple of weeks too.
>
> The truth is the pair of them are fragile and hopelessly intertwined,
> but without tests they're never going to get rewritten, so I might try
> and take some of that on (or get someone here to do it) as I still
> have multiple issues with them - signing doesn't really work the way I
> want, I still have a circular dependency I've not tracked down,
> doubtless other things I can't immediately bring to mind.
>
> If we were to add some tests around it, what would you pull out as the
> simplest thing that it could do which would be valuable?

Actually, you can ignore this patch.  We debugged the actual race 
condition contributing to the confusion, and that developer will submit 
a new patch with that fix.  We can review if that patch makes more sense 
(even without tests).

The test would need to follow:

1) Is there a pub key in the u-boot.dtb (symlinked file) in the deploy 
directory and is it the same as what is generated by mkimage when 
pointing at the fitImage in the deploy directory.
2) The race condition affects the build when there is a cached step 
built incorrectly.   The cached build step injects the u-boot.dtb prior 
to it being signed.

Hopefully that makes sense,

- Mike

>
-- 
Michael Scott
Embedded Software Engineer at Foundries.io
"microPlatforms™ for Connected Products"
E: mike@foundries.io
W: https://www.foundries.io

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[richard.purdie]

On Wed, 2019-06-12 at 19:02 +0100, Alex Kiernan wrote:
> On Wed, Jun 12, 2019 at 10:59 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > On Wed, 2019-06-12 at 09:15 +0100, Alex Kiernan wrote:
> > > Since UBOOT_DTB_BINARY empty means we don't need to inject
> > > signatures
> > > into the U-Boot DTB, we can remove the dependencies between
> > > consumers
> > > of
> > > these two classes and resolve a circular dependency between u-
> > > boot
> > > and
> > > kernel.
> > > 
> > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> > > ---
> > > 
> > >  meta/classes/kernel-fitimage.bbclass | 2 +-
> > >  meta/classes/uboot-sign.bbclass      | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > Really wish we had tests and documentation for these classes...
> > 
> 
> I know... I have a dread anytime anyone touches it, me included... I
> sat on this change for a couple of weeks too.

I also have little idea what to do with the patches, I just don't have
enough information to make any informed decision here :/.

> The truth is the pair of them are fragile and hopelessly intertwined,
> but without tests they're never going to get rewritten, so I might
> try and take some of that on (or get someone here to do it) as I
> still have multiple issues with them - signing doesn't really work
> the way I want, I still have a circular dependency I've not tracked
> down, doubtless other things I can't immediately bring to mind.
> 
> If we were to add some tests around it, what would you pull out as
> the simplest thing that it could do which would be valuable?

I don't know enough about what its doing or how people are using it to
make a useful comment on that.

I'd suggest whoever writes the tests gets to document and test the way
they use it. They can then modify and improve the code such that the
tests keep working. If other untested usecases break, that would be
unfortunate and we'd want to add testcases if they got fixed.

I'd be quite happy to work to such a policy (within reason).

Cheers,

Richard

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[richard.purdie]

On Wed, 2019-06-12 at 14:17 -0700, Michael Scott wrote:
> Actually, you can ignore this patch.  We debugged the actual race 
> condition contributing to the confusion, and that developer will
> submit a new patch with that fix.  We can review if that patch makes
> more sense (even without tests).

Well, this patch merged earlier this evening as tested passed. Is that
a problem?

> The test would need to follow:
> 
> 1) Is there a pub key in the u-boot.dtb (symlinked file) in the
> deploy 
> directory and is it the same as what is generated by mkimage when 
> pointing at the fitImage in the deploy directory.
> 2) The race condition affects the build when there is a cached step 
> built incorrectly.   The cached build step injects the u-boot.dtb
> prior to it being signed.

That does help explain the problem you were seeing and does some like
something we could/should be testing for.

Cheers,

Richard

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Michael Scott]

On 6/12/19 3:21 PM, richard.purdie@linuxfoundation.org wrote:
> On Wed, 2019-06-12 at 14:17 -0700, Michael Scott wrote:
>> Actually, you can ignore this patch.  We debugged the actual race
>> condition contributing to the confusion, and that developer will
>> submit a new patch with that fix.  We can review if that patch makes
>> more sense (even without tests).
> Well, this patch merged earlier this evening as tested passed. Is that
> a problem?

Not a problem at all.  I believe we also kept this change when we 
debugged the actual race condition.

>
>> The test would need to follow:
>>
>> 1) Is there a pub key in the u-boot.dtb (symlinked file) in the
>> deploy
>> directory and is it the same as what is generated by mkimage when
>> pointing at the fitImage in the deploy directory.
>> 2) The race condition affects the build when there is a cached step
>> built incorrectly.   The cached build step injects the u-boot.dtb
>> prior to it being signed.
> That does help explain the problem you were seeing and does some like
> something we could/should be testing for.

Yes if possible the test should run #1 above - 3 ways: 1) clean cache 
for kernel/u-boot/u-boot tools, 2) cached run for kernel/u-boot/u-boot 
tools and 3) cached for kernel/u-boot tools, but changed u-boot 
(invalidate cache?)

- Mike

>
> Cheers,
>
> Richard
>
-- 
Michael Scott
Embedded Software Engineer at Foundries.io
"microPlatforms™ for Connected Products"
E: mike@foundries.io
W: https://www.foundries.io

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Alex Kiernan]

Hi Mike

On Wed, Jun 12, 2019 at 11:59 PM Michael Scott <mike@foundries.io> wrote:
>
>
> On 6/12/19 3:21 PM, richard.purdie@linuxfoundation.org wrote:
> > On Wed, 2019-06-12 at 14:17 -0700, Michael Scott wrote:
> >> Actually, you can ignore this patch.  We debugged the actual race
> >> condition contributing to the confusion, and that developer will
> >> submit a new patch with that fix.  We can review if that patch makes
> >> more sense (even without tests).
> > Well, this patch merged earlier this evening as tested passed. Is that
> > a problem?
>
> Not a problem at all.  I believe we also kept this change when we
> debugged the actual race condition.
>

Wrong patch you're pointing to here? I'm guessing you meant
https://patchwork.openembedded.org/patch/161440/

-- 
Alex Kiernan

Re: [PATCH] kernel-fitimage: uboot-sign: Check UBOOT_DTB_BINARY before adding deps

[Michael Scott]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

Hi Alex and Richard,

On Wed, Jun 12, 2019, 9:06 PM Alex Kiernan <alex.kiernan@gmail.com> wrote:

> Hi Mike
>
> On Wed, Jun 12, 2019 at 11:59 PM Michael Scott <mike@foundries.io> wrote:
> >
> >
> > On 6/12/19 3:21 PM, richard.purdie@linuxfoundation.org wrote:
> > > On Wed, 2019-06-12 at 14:17 -0700, Michael Scott wrote:
> > >> Actually, you can ignore this patch.  We debugged the actual race
> > >> condition contributing to the confusion, and that developer will
> > >> submit a new patch with that fix.  We can review if that patch makes
> > >> more sense (even without tests).
> > > Well, this patch merged earlier this evening as tested passed. Is that
> > > a problem?
> >
> > Not a problem at all.  I believe we also kept this change when we
> > debugged the actual race condition.
> >
>
> Wrong patch you're pointing to here? I'm guessing you meant
> https://patchwork.openembedded.org/patch/161440/


You are 100% correct.  I was checking email on the phone and it trimmed the
subject line.

Please ignore my comments with regard to this patch.

Richard: You are still correct in that the u-boot signing process needs
tests.

My apologies,

- Mike

>
>
> --
> Alex Kiernan
>

[-- Attachment #2: Type: text/html, Size: 2291 bytes --]