| Message ID | 36e996b864853dba26a9c9fb9c9c674e92cc935e.1702555387.git.maria.celeste.cesario@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: address violations of MISRA C:2012 Rule 11.8 | expand |
On 14.12.2023 13:07, Simone Ballarin wrote: > --- a/docs/misra/safe.json > +++ b/docs/misra/safe.json > @@ -28,6 +28,14 @@ > }, > { > "id": "SAF-3-safe", > + "analyser": { > + "eclair": "MC3R1.R11.8" > + }, > + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", > + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." I'm not happy with this description, as it invites for all sorts of abuse. Yet I'm also puzzled that ... > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3413,6 +3413,7 @@ static enum hvm_translation_result __hvm_copy( > enum hvm_translation_result hvm_copy_to_guest_phys( > paddr_t paddr, const void *buf, unsigned int size, struct vcpu *v) > { > + /* SAF-3-safe */ > return __hvm_copy((void *)buf /* HVMCOPY_to_guest doesn't modify */, > paddr, size, v, > HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL); ... this is the only place you then use it. Afaict some of Arm's copy_guest() callers ought to have a similar issue. If so, an enlarged patch should be discussed with a larger audience, to see how we collectively think we want to put this specific kind of deviation. Jan
On Thu, 14 Dec 2023, Jan Beulich wrote: > On 14.12.2023 13:07, Simone Ballarin wrote: > > --- a/docs/misra/safe.json > > +++ b/docs/misra/safe.json > > @@ -28,6 +28,14 @@ > > }, > > { > > "id": "SAF-3-safe", > > + "analyser": { > > + "eclair": "MC3R1.R11.8" > > + }, > > + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", > > + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." > > I'm not happy with this description, as it invites for all sorts of abuse. > Yet I'm also puzzled that ... We can improve the language but the concept would still be the same. For instance: A single function might or might not modify the pointee depending on other function parameters (for instance a single function that could either read or write depending on how it is called). It is safe to cast away const qualifiers when passing a parameter to a function of this type when the other parameters are triggering a read-only operation. > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -3413,6 +3413,7 @@ static enum hvm_translation_result __hvm_copy( > > enum hvm_translation_result hvm_copy_to_guest_phys( > > paddr_t paddr, const void *buf, unsigned int size, struct vcpu *v) > > { > > + /* SAF-3-safe */ > > return __hvm_copy((void *)buf /* HVMCOPY_to_guest doesn't modify */, > > paddr, size, v, > > HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL); > > ... this is the only place you then use it. Afaict some of Arm's copy_guest() > callers ought to have a similar issue. If so, an enlarged patch should be > discussed with a larger audience, to see how we collectively think we want to > put this specific kind of deviation. We have a similar problem, see xen/arch/arm/guestcopy.c raw_copy_to_guest and raw_copy_from_guest. I would use the SAF deviation there too. In the case here, I think the comment "HVMCOPY_to_guest doesn't modify" could be removed as it becomes redundant with SAF-3-safe, but I'll leave it to you.
On 14.12.2023 23:04, Stefano Stabellini wrote: > On Thu, 14 Dec 2023, Jan Beulich wrote: >> On 14.12.2023 13:07, Simone Ballarin wrote: >>> --- a/docs/misra/safe.json >>> +++ b/docs/misra/safe.json >>> @@ -28,6 +28,14 @@ >>> }, >>> { >>> "id": "SAF-3-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R11.8" >>> + }, >>> + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", >>> + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." >> >> I'm not happy with this description, as it invites for all sorts of abuse. >> Yet I'm also puzzled that ... > > We can improve the language but the concept would still be the same. For > instance: > > A single function might or might not modify the pointee depending on > other function parameters (for instance a single function that could > either read or write depending on how it is called). It is safe to cast > away const qualifiers when passing a parameter to a function of this > type when the other parameters are triggering a read-only operation. Right, but I think the next here needs to be setting as tight boundaries as possible: It should cover only this one specific pattern. Anything else would better get its own deviation, imo. >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -3413,6 +3413,7 @@ static enum hvm_translation_result __hvm_copy( >>> enum hvm_translation_result hvm_copy_to_guest_phys( >>> paddr_t paddr, const void *buf, unsigned int size, struct vcpu *v) >>> { >>> + /* SAF-3-safe */ >>> return __hvm_copy((void *)buf /* HVMCOPY_to_guest doesn't modify */, >>> paddr, size, v, >>> HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL); >> >> ... this is the only place you then use it. Afaict some of Arm's copy_guest() >> callers ought to have a similar issue. If so, an enlarged patch should be >> discussed with a larger audience, to see how we collectively think we want to >> put this specific kind of deviation. > > We have a similar problem, see xen/arch/arm/guestcopy.c > raw_copy_to_guest and raw_copy_from_guest. > > I would use the SAF deviation there too. > > In the case here, I think the comment "HVMCOPY_to_guest doesn't modify" > could be removed as it becomes redundant with SAF-3-safe, but I'll leave > it to you. No, the comment cannot be removed: The SAF comment says exactly nothing until you go and look up its description. The two comments could be folded, though. Which is something I was trying to advocate for in general: Unless entirely obvious, what exactly it is that is "safe" would better be (briefly) stated in these SAF comments. Jan
On Fri, 15 Dec 2023, Jan Beulich wrote: > On 14.12.2023 23:04, Stefano Stabellini wrote: > > On Thu, 14 Dec 2023, Jan Beulich wrote: > >> On 14.12.2023 13:07, Simone Ballarin wrote: > >>> --- a/docs/misra/safe.json > >>> +++ b/docs/misra/safe.json > >>> @@ -28,6 +28,14 @@ > >>> }, > >>> { > >>> "id": "SAF-3-safe", > >>> + "analyser": { > >>> + "eclair": "MC3R1.R11.8" > >>> + }, > >>> + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", > >>> + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." > >> > >> I'm not happy with this description, as it invites for all sorts of abuse. > >> Yet I'm also puzzled that ... > > > > We can improve the language but the concept would still be the same. For > > instance: > > > > A single function might or might not modify the pointee depending on > > other function parameters (for instance a single function that could > > either read or write depending on how it is called). It is safe to cast > > away const qualifiers when passing a parameter to a function of this > > type when the other parameters are triggering a read-only operation. > > Right, but I think the next here needs to be setting as tight boundaries > as possible: It should cover only this one specific pattern. Anything > else would better get its own deviation, imo. OK. What about: A single function might or might not modify the pointee depending on other function parameters, for instance a common pattern is to implement one function that could either read or write depending on how it is called. It is safe to cast away const qualifiers when passing a parameter to a function following this pattern when the other parameters are triggering a read-only operation. Feel free to suggest a better wording. > >>> --- a/xen/arch/x86/hvm/hvm.c > >>> +++ b/xen/arch/x86/hvm/hvm.c > >>> @@ -3413,6 +3413,7 @@ static enum hvm_translation_result __hvm_copy( > >>> enum hvm_translation_result hvm_copy_to_guest_phys( > >>> paddr_t paddr, const void *buf, unsigned int size, struct vcpu *v) > >>> { > >>> + /* SAF-3-safe */ > >>> return __hvm_copy((void *)buf /* HVMCOPY_to_guest doesn't modify */, > >>> paddr, size, v, > >>> HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL); > >> > >> ... this is the only place you then use it. Afaict some of Arm's copy_guest() > >> callers ought to have a similar issue. If so, an enlarged patch should be > >> discussed with a larger audience, to see how we collectively think we want to > >> put this specific kind of deviation. > > > > We have a similar problem, see xen/arch/arm/guestcopy.c > > raw_copy_to_guest and raw_copy_from_guest. > > > > I would use the SAF deviation there too. > > > > In the case here, I think the comment "HVMCOPY_to_guest doesn't modify" > > could be removed as it becomes redundant with SAF-3-safe, but I'll leave > > it to you. > > No, the comment cannot be removed: The SAF comment says exactly nothing > until you go and look up its description. The two comments could be > folded, though. Which is something I was trying to advocate for in > general: Unless entirely obvious, what exactly it is that is "safe" > would better be (briefly) stated in these SAF comments. I agree with you
On 15.12.2023 22:02, Stefano Stabellini wrote: > On Fri, 15 Dec 2023, Jan Beulich wrote: >> On 14.12.2023 23:04, Stefano Stabellini wrote: >>> On Thu, 14 Dec 2023, Jan Beulich wrote: >>>> On 14.12.2023 13:07, Simone Ballarin wrote: >>>>> --- a/docs/misra/safe.json >>>>> +++ b/docs/misra/safe.json >>>>> @@ -28,6 +28,14 @@ >>>>> }, >>>>> { >>>>> "id": "SAF-3-safe", >>>>> + "analyser": { >>>>> + "eclair": "MC3R1.R11.8" >>>>> + }, >>>>> + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", >>>>> + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." >>>> >>>> I'm not happy with this description, as it invites for all sorts of abuse. >>>> Yet I'm also puzzled that ... >>> >>> We can improve the language but the concept would still be the same. For >>> instance: >>> >>> A single function might or might not modify the pointee depending on >>> other function parameters (for instance a single function that could >>> either read or write depending on how it is called). It is safe to cast >>> away const qualifiers when passing a parameter to a function of this >>> type when the other parameters are triggering a read-only operation. >> >> Right, but I think the next here needs to be setting as tight boundaries >> as possible: It should cover only this one specific pattern. Anything >> else would better get its own deviation, imo. > > OK. What about: > > A single function might or might not modify the pointee depending on > other function parameters, for instance a common pattern is to implement > one function that could either read or write depending on how it is > called. It is safe to cast away const qualifiers when passing a > parameter to a function following this pattern when the other parameters > are triggering a read-only operation. > > Feel free to suggest a better wording. Well, my point was to get rid of "for instance" and "common pattern" (and anything alike). E.g.: "A single function could either read or write through a passed in pointer, depending on how it is called. It is deemed safe to cast away a const qualifier when passing a pointer to such a function, when the other parameters guarantee read-only operation." Jan
On Mon, 18 Dec 2023, Jan Beulich wrote: > On 15.12.2023 22:02, Stefano Stabellini wrote: > > On Fri, 15 Dec 2023, Jan Beulich wrote: > >> On 14.12.2023 23:04, Stefano Stabellini wrote: > >>> On Thu, 14 Dec 2023, Jan Beulich wrote: > >>>> On 14.12.2023 13:07, Simone Ballarin wrote: > >>>>> --- a/docs/misra/safe.json > >>>>> +++ b/docs/misra/safe.json > >>>>> @@ -28,6 +28,14 @@ > >>>>> }, > >>>>> { > >>>>> "id": "SAF-3-safe", > >>>>> + "analyser": { > >>>>> + "eclair": "MC3R1.R11.8" > >>>>> + }, > >>>>> + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", > >>>>> + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." > >>>> > >>>> I'm not happy with this description, as it invites for all sorts of abuse. > >>>> Yet I'm also puzzled that ... > >>> > >>> We can improve the language but the concept would still be the same. For > >>> instance: > >>> > >>> A single function might or might not modify the pointee depending on > >>> other function parameters (for instance a single function that could > >>> either read or write depending on how it is called). It is safe to cast > >>> away const qualifiers when passing a parameter to a function of this > >>> type when the other parameters are triggering a read-only operation. > >> > >> Right, but I think the next here needs to be setting as tight boundaries > >> as possible: It should cover only this one specific pattern. Anything > >> else would better get its own deviation, imo. > > > > OK. What about: > > > > A single function might or might not modify the pointee depending on > > other function parameters, for instance a common pattern is to implement > > one function that could either read or write depending on how it is > > called. It is safe to cast away const qualifiers when passing a > > parameter to a function following this pattern when the other parameters > > are triggering a read-only operation. > > > > Feel free to suggest a better wording. > > Well, my point was to get rid of "for instance" and "common pattern" (and > anything alike). E.g.: > > "A single function could either read or write through a passed in pointer, > depending on how it is called. It is deemed safe to cast away a const > qualifier when passing a pointer to such a function, when the other > parameters guarantee read-only operation." That's fine by me
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index 952324f85c..e748bc6cf5 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -28,6 +28,14 @@ }, { "id": "SAF-3-safe", + "analyser": { + "eclair": "MC3R1.R11.8" + }, + "name": "MC3R1.R11.8: removal of const qualifier to comply with function signature", + "text": "It is safe to cast away const qualifiers to comply with function signature if the function does not modify the pointee." + }, + { + "id": "SAF-4-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 523e0df57c..414853254f 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3413,6 +3413,7 @@ static enum hvm_translation_result __hvm_copy( enum hvm_translation_result hvm_copy_to_guest_phys( paddr_t paddr, const void *buf, unsigned int size, struct vcpu *v) { + /* SAF-3-safe */ return __hvm_copy((void *)buf /* HVMCOPY_to_guest doesn't modify */, paddr, size, v, HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL);