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

[Ondrej Kozina <okozina@redhat.com>]

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.