Hi Raghu and Louis,
On 4/7/20 12:14 PM, Louis Mayencourt via TF-A wrote:
I do agree with you: case 2 and 3 are similar (wrongly formed DTB) and should lead to the same behavior.
A mandatory property miss or a hit with a structurally incorrect node means that the DTB doesn't follow the provided binding document. Such a DTB shouldn't be considered as valid and should trigger a build failure and/or a code panic.
That's what still confuses me... Agree on cases 2 and 3 triggering a build failure if possible, but not a code panic. A code panic stays in a release build. With what we've been discussing so far, it would seem more appropriate to me to have debug assertions to catch cases 2 and 3. These debug assertions can help catching structural problems in the DTB during the development phase and can be eliminated for a production build, leaving no checks whatsoever in the code.
This is the strategy we've been using so far in TF-A. For lots of platform interfaces, the generic code includes debug assertions to check the correct implementation of these interfaces by platform integrators. For example, checking the range of their return values. I would say this is deeply embedded into the threat model TF-A uses today. See https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...
On one hand, it makes sense to me. On the other hand, I take Raghu's point that it would be unrealistic to assume that 100% of code has been covered by tests. This is very hard to achieve in practice, especially to cover all error cases ; thus, it seems utopian to assume that all debug assertions have been exercised during development and can be safely removed.
TF-A does provide a way to keep debug assertions in a release build (using the ENABLE_ASSERTIONS build flag) if platform integrators judge they would rather keep them but this is not the default behaviour.
Regards, Sandrine
Thanks Sandrine, Louis,
Agree with both of you. I'm fine with using asserts or panics, as long as we uniformly use it. The change i sent out(review 3845) was because i noticed inconsistency in handling errors. If we do use asserts, all code that checks for mandatory properties, should be changed to assert as opposed to return error code. For optional properties, we can continue to return an error code or print warnings etc.
I would like to point out that using asserts here is different from what is documented in the coding guidelines. The guidelines explicitly spells out using asserts for "programming errors". Now, is having a bad DTB considered a programming error? ;) The DTB is platform data as opposed to code. The guidelines might need to be updated based on the conclusion here. Also note the couple of scenarios i mentioned in an earlier email. Platforms like RPI4 don't generally enable TBBR and the DTB image could get corrupt or be modified on purpose. On a release build, this could cause silent corruption. Panic() would avoid this.
In any case, it would be good to settle on the expected behavior for each of these abnormal cases. I don't have a strong preference for asserts or panics here, since each has its pros and cons as both of you called out.
Thanks Raghu
On 4/7/20 3:34 AM, Sandrine Bailleux wrote:
Hi Raghu and Louis,
On 4/7/20 12:14 PM, Louis Mayencourt via TF-A wrote:
I do agree with you: case 2 and 3 are similar (wrongly formed DTB) and should lead to the same behavior.
A mandatory property miss or a hit with a structurally incorrect node means that the DTB doesn't follow the provided binding document. Such a DTB shouldn't be considered as valid and should trigger a build failure and/or a code panic.
That's what still confuses me... Agree on cases 2 and 3 triggering a build failure if possible, but not a code panic. A code panic stays in a release build. With what we've been discussing so far, it would seem more appropriate to me to have debug assertions to catch cases 2 and 3. These debug assertions can help catching structural problems in the DTB during the development phase and can be eliminated for a production build, leaving no checks whatsoever in the code.
This is the strategy we've been using so far in TF-A. For lots of platform interfaces, the generic code includes debug assertions to check the correct implementation of these interfaces by platform integrators. For example, checking the range of their return values. I would say this is deeply embedded into the threat model TF-A uses today. See https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...
On one hand, it makes sense to me. On the other hand, I take Raghu's point that it would be unrealistic to assume that 100% of code has been covered by tests. This is very hard to achieve in practice, especially to cover all error cases ; thus, it seems utopian to assume that all debug assertions have been exercised during development and can be safely removed.
TF-A does provide a way to keep debug assertions in a release build (using the ENABLE_ASSERTIONS build flag) if platform integrators judge they would rather keep them but this is not the default behaviour.
Regards, Sandrine
Hi Raghu,
On 4/8/20 12:50 AM, Raghu Krishnamurthy wrote:
Thanks Sandrine, Louis,
Agree with both of you. I'm fine with using asserts or panics, as long as we uniformly use it. The change i sent out(review 3845) was because i noticed inconsistency in handling errors. If we do use asserts, all code that checks for mandatory properties, should be changed to assert as opposed to return error code. For optional properties, we can continue to return an error code or print warnings etc.
Yes, I too think consistency is key here. As you said, we need to settle on the expected behaviour and enforce it uniformly across the fconf code. Let's revisit this code after the v2.3 release.
I would like to point out that using asserts here is different from what is documented in the coding guidelines. The guidelines explicitly spells out using asserts for "programming errors". Now, is having a bad DTB considered a programming error? ;) The DTB is platform data as opposed to code. The guidelines might need to be updated based on the conclusion here.
Now that you point it out, and after taking a closer look at [1], I think I was wrong. A bad DTB seems more like an integration error than a programming error to me, and thus should be handled with runtime checks according to the current guidelines. Incorrect values for mandatory properties should probably be treated as unrecoverable errors (causing a panic) and incorrect/missing optional properties as recoverable errors (issuing a warning message).
[1] https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...
Also note the couple of scenarios i mentioned in an earlier email. Platforms like RPI4 don't generally enable TBBR and the DTB image could get corrupt or be modified on purpose. On a release build, this could cause silent corruption. Panic() would avoid this.
In any case, it would be good to settle on the expected behavior for each of these abnormal cases. I don't have a strong preference for asserts or panics here, since each has its pros and cons as both of you called out.
Thanks Raghu
Thanks Sandrine. Revisiting after v2.3 is sounds fine.
-Raghu
On 4/10/20 2:25 AM, Sandrine Bailleux wrote:
Hi Raghu,
On 4/8/20 12:50 AM, Raghu Krishnamurthy wrote:
Thanks Sandrine, Louis,
Agree with both of you. I'm fine with using asserts or panics, as long as we uniformly use it. The change i sent out(review 3845) was because i noticed inconsistency in handling errors. If we do use asserts, all code that checks for mandatory properties, should be changed to assert as opposed to return error code. For optional properties, we can continue to return an error code or print warnings etc.
Yes, I too think consistency is key here. As you said, we need to settle on the expected behaviour and enforce it uniformly across the fconf code. Let's revisit this code after the v2.3 release.
I would like to point out that using asserts here is different from what is documented in the coding guidelines. The guidelines explicitly spells out using asserts for "programming errors". Now, is having a bad DTB considered a programming error? ;) The DTB is platform data as opposed to code. The guidelines might need to be updated based on the conclusion here.
Now that you point it out, and after taking a closer look at [1], I think I was wrong. A bad DTB seems more like an integration error than a programming error to me, and thus should be handled with runtime checks according to the current guidelines. Incorrect values for mandatory properties should probably be treated as unrecoverable errors (causing a panic) and incorrect/missing optional properties as recoverable errors (issuing a warning message).
[1] https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...
Also note the couple of scenarios i mentioned in an earlier email. Platforms like RPI4 don't generally enable TBBR and the DTB image could get corrupt or be modified on purpose. On a release build, this could cause silent corruption. Panic() would avoid this.
In any case, it would be good to settle on the expected behavior for each of these abnormal cases. I don't have a strong preference for asserts or panics here, since each has its pros and cons as both of you called out.
Thanks Raghu
Hi All,
Since the 2.3 release is done, can we revisit this topic? I think we left this with finalizing on how to handle bad/incorrect DTB's. Sandrine's latest proposal below:
"A bad DTB seems more like an integration error than a programming error to me, and thus should be handled with runtime checks according to the current guidelines. Incorrect values for mandatory properties should probably be treated as unrecoverable errors (causing a panic) and incorrect/missing optional properties as recoverable errors (issuing a warning message)."
I agree with this proposal and think we should follow this. This addresses my original concern of handling errors consistently and being safe by verifying mandatory/critical properties at run-time.
Thoughts ?
Thanks Raghu
On 4/13/20 10:27 AM, Raghu Krishnamurthy wrote:
Thanks Sandrine. Revisiting after v2.3 is sounds fine.
-Raghu
On 4/10/20 2:25 AM, Sandrine Bailleux wrote:
Hi Raghu,
On 4/8/20 12:50 AM, Raghu Krishnamurthy wrote:
Thanks Sandrine, Louis,
Agree with both of you. I'm fine with using asserts or panics, as long as we uniformly use it. The change i sent out(review 3845) was because i noticed inconsistency in handling errors. If we do use asserts, all code that checks for mandatory properties, should be changed to assert as opposed to return error code. For optional properties, we can continue to return an error code or print warnings etc.
Yes, I too think consistency is key here. As you said, we need to settle on the expected behaviour and enforce it uniformly across the fconf code. Let's revisit this code after the v2.3 release.
I would like to point out that using asserts here is different from what is documented in the coding guidelines. The guidelines explicitly spells out using asserts for "programming errors". Now, is having a bad DTB considered a programming error? ;) The DTB is platform data as opposed to code. The guidelines might need to be updated based on the conclusion here.
Now that you point it out, and after taking a closer look at [1], I think I was wrong. A bad DTB seems more like an integration error than a programming error to me, and thus should be handled with runtime checks according to the current guidelines. Incorrect values for mandatory properties should probably be treated as unrecoverable errors (causing a panic) and incorrect/missing optional properties as recoverable errors (issuing a warning message).
[1] https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...
Also note the couple of scenarios i mentioned in an earlier email. Platforms like RPI4 don't generally enable TBBR and the DTB image could get corrupt or be modified on purpose. On a release build, this could cause silent corruption. Panic() would avoid this.
In any case, it would be good to settle on the expected behavior for each of these abnormal cases. I don't have a strong preference for asserts or panics here, since each has its pros and cons as both of you called out.
Thanks Raghu
tf-a@lists.trustedfirmware.org
-
Raghu K -
Raghu Krishnamurthy
-
Sandrine Bailleux