Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2023 18:07, Julien Grall wrote:
> Hi Jan,
> 
> On 23/10/2023 16:15, Jan Beulich wrote:
>> On 23.10.2023 17:00, Julien Grall wrote:
>>>
>>>
>>> On 23/10/2023 15:51, Nicola Vetrini wrote:
>>>> Hi,
>>>
>>> Hi Nicola,
>>>
>>>> while taking care of some patches regarding MISRA C Rule 2.1 (code
>>>> shouldn't be unreachable), I
>>>> came across this function:
>>>>
>>>> long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>> {
>>>>       switch ( domctl->cmd )
>>>>       {
>>>>       case XEN_DOMCTL_set_address_size:
>>>>           switch ( domctl->u.address_size.size )
>>>>           {
>>>>           case 32:
>>>>               if ( !cpu_has_el1_32 )
>>>>                   return -EINVAL;
>>>>               /* SVE is not supported for 32 bit domain */
>>>>               if ( is_sve_domain(d) )
>>>>                   return -EINVAL;
>>>>               return switch_mode(d, DOMAIN_32BIT);
>>>>           case 64:
>>>>               return switch_mode(d, DOMAIN_64BIT);
>>>>           default:
>>>>               return -EINVAL;
>>>>           }
>>>>           break;
>>>>
>>>>       default:
>>>>           return -ENOSYS;
>>>>       }
>>>> }
>>>>
>>>> here the break after the innermost switch is clearly unreachable, but
>>>> it's also guarding a possible fallthrough.
>>>> I can see a couple of solutions to this:
>>>>
>>>> - mark the part after the switch unreachable;
>>>> - introduce a variable 'long rc' to store the return value, and
>>>> consequently rework the control flow of all the switches
>>>>     (e.g. rc = -EINVAL and similar);
>>>> - remove the break, but I consider this a risky move, unless -ENOSYS
>>>> would be an ok value to be returned if some case
>>>>     from the switch above does not have a return statement.
>>>
>>> - move the nested switch in a separate function, so the code in
>>> subarch_do_domctl() can be replaced with:
>>>
>>> return set_address_size(...);
>>
>> But that would help only if inside the new function you still re-
>> layout the switch() (or replace it by, say, if/else-if/else),
>> wouldn't it?
> 
> I am not sure why I would need to re-layout the switch. This should work 
> (untested):
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 14fc622e9956..8720d126c97d 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum 
> domain_type type)
>       return 0;
>   }
> 
> +static long set_address_size(struct domain *d, uint32_t address_size)
> +{
> +    switch ( address_size )
> +    {
> +    case 32:
> +        if ( !cpu_has_el1_32 )
> +            return -EINVAL;
> +        /* SVE is not supported for 32 bit domain */
> +        if ( is_sve_domain(d) )
> +            return -EINVAL;
> +        return switch_mode(d, DOMAIN_32BIT);
> +    case 64:
> +        return switch_mode(d, DOMAIN_64BIT);
> +    default:
> +        return -EINVAL;
> +    }
> +}

Well, yes, if you're happy to have a function returning a value without
a return statement at its end.

Jan