Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling <walling@linux.ibm.com>]

On 7/20/20 4:17 AM, David Hildenbrand wrote:
> On 24.06.20 22:23, Collin Walling wrote:
>> Rework the SCLP boundary check to account for different SCLP commands
>> (eventually) allowing different boundary sizes.
>>
>> Move the length check code into a separate function, and introduce a
>> new function to determine the length of the read SCP data (i.e. the size
>> from the start of the struct to where the CPU entries should begin).
>>
>> The format of read CPU info is unlikely to change in the future,
>> so we do not require a separate function to calculate its length.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 44 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 181ce04007..5899c1e3b8 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>>      return false;
>>  }
>>  
>> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
>> +                                 SCCBHeader *header)
>> +{
>> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1;
>> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>> +
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    default:
>> +        if (sccb_max_addr < sccb_boundary) {
>> +            return true;
>> +        }
>> +    }
> 
> ^ what is that?
> 
>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>         return true;
>     }
> 

I agree it looks pointless in this patch, but it makes more sense in
patch #6 where we introduce cases for the SCLP commands that bypass
these checks if the extended-length sccb feature is enabled.

>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    return false;
> 
> So we return "false" on success? At least I consider that weird when
> returning the bool type. Maybe make it clearer what the function indicates
> 

Hmmm... I figured since there were more paths that can lead to success
(i.e. when I introduce the feat check in a later patch), then it made
more sense to to return false at the end. sclp_command_code_valid has
similar logic.

But if boolean functions traditionally return true as the last return
value, I can rework it to align to coding preferences / standards.

> "sccb_boundary_is_invalid"
> 

Unless it's simply the name that is confusing?

> or leave it named as is and switch from return value "bool" to "int",
> using "0" on success and "-EINVAL" on error.
> 

Is the switch statement an overkill? I thought of it as a cleaner way to
later show which commands have a special conditions (introduced in patch
6 for the ELS stuff) instead of a nasty long if statement.

The alternative...

/* Comment explaining this check */
if ((code & SCLP_CMD_CODE_MASK) & (SCLP_CMDW_READ_SCP_INFO |
	SCLP_CMDW_READ_SCP_INFO_FORCED | SCLP_CMDW_READ_CPU_INFO) &&
	s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
	return true;
}

if (sccb_max_addr < sccb_boundary) {
        return true;
}

header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
return false;

[...]

-- 
Regards,
Collin

Stay safe and stay healthy