Re: [PATCH 5/5] sed-opal: Add command to read locking range parameters.

Ondrej Kozina
2023-04-05T09:40:42Z

On 05. 04. 23 10:27, Christian Brauner wrote:
> On Wed, Mar 22, 2023 at 04:16:04PM +0100, Ondrej Kozina wrote:
>> + if (!response_token_matches(tok, OPAL_STARTNAME)) {
>> + pr_debug("Unexpected response token type %d.\n", n);
>> + return OPAL_INVAL_PARAM;
>> + }
>> +
>> + if (response_get_u64(resp, ++n) != column) {
>
> Please don't rely on side-effects and increment explicitly before or
> after the functin call so ++n and n++ doesn't matter.

Going to fix in version 2.

>
>> + pr_debug("Token %d does not match expected column %u.\n", n, column);
>> + return OPAL_INVAL_PARAM;
>> + }
>> +
>> + val = response_get_u64(resp, ++n);
>> +
>> + tok = response_get_token(resp, ++n);
>> + if (IS_ERR(tok))
>> + return PTR_ERR(tok);
>> +
>> + if (!response_token_matches(tok, OPAL_ENDNAME)) {
>> + pr_debug("Unexpected response token type %d.\n", n);
>> + return OPAL_INVAL_PARAM;
>> + }
>> +
>> + *value = val;
>> + *iter = ++n;
>
> This is how they explain side-effects in textbooks. :)

Ditto.

(...)
>> +
>> + /* skip session info when copying back to uspace */
>> + if (!ret && copy_to_user(data + offsetof(struct opal_lr_status, range_start),
>> + (void *)opal_lrst + offsetof(struct opal_lr_status, range_start),
>
> Better written as
>
> (void *)(opal_lrst + offsetof(struct opal_lr_status, range_start))

Nack. I need to read bytes from offset _inside_ struct opal_lr_status.
This change would actually read from memory beyond pointed to by
opal_lrst pointer.

(...)
>> diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
>> index d7a1524023db..3905c8ffedbf 100644
>> --- a/include/uapi/linux/sed-opal.h
>> +++ b/include/uapi/linux/sed-opal.h
>> @@ -78,6 +78,16 @@ struct opal_user_lr_setup {
>> struct opal_session_info session;
>> };
>>
>> +struct opal_lr_status {
>> + struct opal_session_info session;
>> + __u64 range_start;
>> + __u64 range_length;
>> + __u32 RLE; /* Read Lock enabled */
>> + __u32 WLE; /* Write Lock Enabled */
>
> Why is that in capital letters if I may ask? That seems strange uapi for
> Linux. And why not just "read_lock_enabled" and "write_lock_enabled"
> given that we also have "range_start" and "range_length". Let's not
> CREAT one of those weird uapis if we don't have to.

See 'opal_user_lr_setup' struct above. Since the new command is supposed
to return those parameters I did not want to add confusion by naming it
differently.

>
>> + __u32 l_state;
>
> "locking_state"?

Same as above, see 'opal_lock_unlock' struct. It's even spicier
considering it's impossible to set WRITE_ONLY state (lock only read I/O)
with sed-opal iface.