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>
---
Changes in v3:
- do not return 0 (success).

Al least this version returns an error code but I am not sure about
which one to use.
---
 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 648d6dd475..a1e28b3360 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -916,6 +916,7 @@ get_page_from_l1e(
                 return 0;
             default:
                 ASSERT_UNREACHABLE();
+                return -EPERM;
             }
         }
         else if ( l1f & _PAGE_RW )
-- 
2.34.1

On 26.06.2024 11:28, 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>

Tentatively
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -916,6 +916,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return -EPERM;
>              }
>          }
>          else if ( l1f & _PAGE_RW )

I don't like the use of -EPERM here very much, but I understand that there's
no really suitable errno value. I wonder though whether something far more
"exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
Just to mention it: -EPERM is what failed XSM checks would typically yield,
so from that perspective alone even switching to -EACCES might be a little
bit better.

I further wonder whether, with the assertion catching an issue with the
implementation, we shouldn't consider using BUG() here instead. Input from
in particular the other x86 maintainers appreciated.

Jan

On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote:
> On 26.06.2024 11:28, 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>
>
> Tentatively
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -916,6 +916,7 @@ get_page_from_l1e(
> >                  return 0;
> >              default:
> >                  ASSERT_UNREACHABLE();
> > +                return -EPERM;
> >              }
> >          }
> >          else if ( l1f & _PAGE_RW )
>
> I don't like the use of -EPERM here very much, but I understand that there's
> no really suitable errno value. I wonder though whether something far more
> "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
> Just to mention it: -EPERM is what failed XSM checks would typically yield,
> so from that perspective alone even switching to -EACCES might be a little
> bit better.
>

fwiw: EACCES, being typically used for interface version mismatches, would
confuse me a lot.

> I further wonder whether, with the assertion catching an issue with the
> implementation, we shouldn't consider using BUG() here instead. Input from
> in particular the other x86 maintainers appreciated.
>
> Jan

Cheers,
Alejandro

On 09.07.2024 13:21, Alejandro Vallejo wrote:
> On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote:
>> On 26.06.2024 11:28, Federico Serafini wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -916,6 +916,7 @@ get_page_from_l1e(
>>>                  return 0;
>>>              default:
>>>                  ASSERT_UNREACHABLE();
>>> +                return -EPERM;
>>>              }
>>>          }
>>>          else if ( l1f & _PAGE_RW )
>>
>> I don't like the use of -EPERM here very much, but I understand that there's
>> no really suitable errno value. I wonder though whether something far more
>> "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
>> Just to mention it: -EPERM is what failed XSM checks would typically yield,
>> so from that perspective alone even switching to -EACCES might be a little
>> bit better.
> 
> fwiw: EACCES, being typically used for interface version mismatches, would
> confuse me a lot.

There's no interface version check anywhere in hypercalls involving
get_page_from_l1e(), I don't think. So I see little room for confusion.

Jan