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.
