1. Patchew
  2. Xen
  3. [XEN PATCH v5 0/8] x86: address some violations of MISRA C Rule 16.3
  4. View patch

[XEN PATCH v5 7/8] x86/mm: add defensive return

Federico Serafini posted 8 patches 1 month, 3 weeks ago
[XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Federico Serafini 1 month, 3 weeks ago 
Add defensive return statement at the end of an unreachable
default case. Other than improve safety, this meets the requirements
to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
statement shall terminate every switch-clause".

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
No changes from v3 and v4, further feedback on this thread would be appreciated:
https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
---
 xen/arch/x86/mm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 95795567f2..8973e76dff 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -917,6 +917,7 @@ get_page_from_l1e(
                 return 0;
             default:
                 ASSERT_UNREACHABLE();
+                return -EPERM;
             }
         }
         else if ( l1f & _PAGE_RW )
-- 
2.34.1
Re: [XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Stefano Stabellini 1 month, 3 weeks ago 
On Mon, 29 Jul 2024, Federico Serafini wrote:
> Add defensive return statement at the end of an unreachable
> default case. Other than improve safety, this meets the requirements
> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
> No changes from v3 and v4, further feedback on this thread would be appreciated:
> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> ---
>  xen/arch/x86/mm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 95795567f2..8973e76dff 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -917,6 +917,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return -EPERM;
>              }
>          }
>          else if ( l1f & _PAGE_RW )
> -- 
> 2.34.1
> 
>
Re: [XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Stefano Stabellini 2 weeks, 6 days ago 
On Mon, 29 Jul 2024, Stefano Stabellini wrote:
> On Mon, 29 Jul 2024, Federico Serafini wrote:
> > Add defensive return statement at the end of an unreachable
> > default case. Other than improve safety, this meets the requirements
> > to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> > statement shall terminate every switch-clause".
> > 
> > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> > ---
> > No changes from v3 and v4, further feedback on this thread would be appreciated:
> > https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html

Looking at the older threads, I looks like Jan suggested EACCES, I also
think it is marginally better. My R-b stands also for EACCES. Jan, I
think you should go ahead and fix on commit


> > ---
> >  xen/arch/x86/mm.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 95795567f2..8973e76dff 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -917,6 +917,7 @@ get_page_from_l1e(
> >                  return 0;
> >              default:
> >                  ASSERT_UNREACHABLE();
> > +                return -EPERM;
> >              }
> >          }
> >          else if ( l1f & _PAGE_RW )
> > -- 
> > 2.34.1
> > 
> > 
>
Re: [XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Jan Beulich 2 weeks, 6 days ago 
On 29.08.2024 02:35, Stefano Stabellini wrote:
> On Mon, 29 Jul 2024, Stefano Stabellini wrote:
>> On Mon, 29 Jul 2024, Federico Serafini wrote:
>>> Add defensive return statement at the end of an unreachable
>>> default case. Other than improve safety, this meets the requirements
>>> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
>>> statement shall terminate every switch-clause".
>>>
>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>>> ---
>>> No changes from v3 and v4, further feedback on this thread would be appreciated:
>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> 
> Looking at the older threads, I looks like Jan suggested EACCES, I also
> think it is marginally better. My R-b stands also for EACCES. Jan, I
> think you should go ahead and fix on commit

No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
suggestion for the error code to use by anyone. After all, as you confirm,
EACCES is only marginally better.

Jan
Re: [XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Roger Pau Monné 2 weeks, 5 days ago 
On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote:
> On 29.08.2024 02:35, Stefano Stabellini wrote:
> > On Mon, 29 Jul 2024, Stefano Stabellini wrote:
> >> On Mon, 29 Jul 2024, Federico Serafini wrote:
> >>> Add defensive return statement at the end of an unreachable
> >>> default case. Other than improve safety, this meets the requirements
> >>> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> >>> statement shall terminate every switch-clause".
> >>>
> >>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> >>
> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >>> ---
> >>> No changes from v3 and v4, further feedback on this thread would be appreciated:
> >>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> > 
> > Looking at the older threads, I looks like Jan suggested EACCES, I also
> > think it is marginally better. My R-b stands also for EACCES. Jan, I
> > think you should go ahead and fix on commit
> 
> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
> suggestion for the error code to use by anyone. After all, as you confirm,
> EACCES is only marginally better.

Hm, the only alternative I could suggest is using ERANGE, to signal
the value of opt_mmio_relax is out of the expected range, however that
could be confusing for the callers?

One benefit of using ERANGE is that there's currently no return path
in get_page_from_l1e() with that error code, so it would be very easy
to spot when an unexpected value of opt_mmio_relax is found.  However
there are no guarantees that further error paths might use that error
code.

Thanks, Roger.
Re: [XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Andrew Cooper 2 weeks, 5 days ago 
On 30/08/2024 10:18 am, Roger Pau Monné wrote:
> On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote:
>> On 29.08.2024 02:35, Stefano Stabellini wrote:
>>> On Mon, 29 Jul 2024, Stefano Stabellini wrote:
>>>> On Mon, 29 Jul 2024, Federico Serafini wrote:
>>>>> Add defensive return statement at the end of an unreachable
>>>>> default case. Other than improve safety, this meets the requirements
>>>>> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
>>>>> statement shall terminate every switch-clause".
>>>>>
>>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>>> ---
>>>>> No changes from v3 and v4, further feedback on this thread would be appreciated:
>>>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
>>> Looking at the older threads, I looks like Jan suggested EACCES, I also
>>> think it is marginally better. My R-b stands also for EACCES. Jan, I
>>> think you should go ahead and fix on commit
>> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
>> suggestion for the error code to use by anyone. After all, as you confirm,
>> EACCES is only marginally better.
> Hm, the only alternative I could suggest is using ERANGE, to signal
> the value of opt_mmio_relax is out of the expected range, however that
> could be confusing for the callers?
>
> One benefit of using ERANGE is that there's currently no return path
> in get_page_from_l1e() with that error code, so it would be very easy
> to spot when an unexpected value of opt_mmio_relax is found.  However
> there are no guarantees that further error paths might use that error
> code.

EPERM and EACCES are both very wrong here.  They imply something that is
simply not appropriate in this context.

We use EILSEQ elsewhere for believed-impossible conditions where we need
an errno of some kind.  I suggest we use it here too.

~Andrew
Re: [XEN PATCH v5 7/8] x86/mm: add defensive return
Posted by Roger Pau Monné 2 weeks, 2 days ago 
On Fri, Aug 30, 2024 at 08:17:40PM +0100, Andrew Cooper wrote:
> On 30/08/2024 10:18 am, Roger Pau Monné wrote:
> > On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote:
> >> On 29.08.2024 02:35, Stefano Stabellini wrote:
> >>> On Mon, 29 Jul 2024, Stefano Stabellini wrote:
> >>>> On Mon, 29 Jul 2024, Federico Serafini wrote:
> >>>>> Add defensive return statement at the end of an unreachable
> >>>>> default case. Other than improve safety, this meets the requirements
> >>>>> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> >>>>> statement shall terminate every switch-clause".
> >>>>>
> >>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>>>
> >>>>> ---
> >>>>> No changes from v3 and v4, further feedback on this thread would be appreciated:
> >>>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> >>> Looking at the older threads, I looks like Jan suggested EACCES, I also
> >>> think it is marginally better. My R-b stands also for EACCES. Jan, I
> >>> think you should go ahead and fix on commit
> >> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
> >> suggestion for the error code to use by anyone. After all, as you confirm,
> >> EACCES is only marginally better.
> > Hm, the only alternative I could suggest is using ERANGE, to signal
> > the value of opt_mmio_relax is out of the expected range, however that
> > could be confusing for the callers?
> >
> > One benefit of using ERANGE is that there's currently no return path
> > in get_page_from_l1e() with that error code, so it would be very easy
> > to spot when an unexpected value of opt_mmio_relax is found.  However
> > there are no guarantees that further error paths might use that error
> > code.
> 
> EPERM and EACCES are both very wrong here.  They imply something that is
> simply not appropriate in this context.
> 
> We use EILSEQ elsewhere for believed-impossible conditions where we need
> an errno of some kind.  I suggest we use it here too.

Fine with me.  With that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.