| Message ID | 20230322151604.401680-6-okozina@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sed-opal: add command to read locking range attributes | expand |
On Wed, Mar 22, 2023 at 04:16:04PM +0100, Ondrej Kozina wrote:
> +static int response_get_column(const struct parsed_resp *resp, int *iter, u8 column, u64 *value)
Please avoid the overly long line (plus a few more later).
Otherwise this looks good to me.
On Wed, Mar 22, 2023 at 04:16:04PM +0100, Ondrej Kozina wrote: > It returns following attributes: > > locking range start > locking range length > read lock enabled > write lock enabled > lock state (RW, RO or LK) > > It can be retrieved by user authority provided the authority > was added to locking range via prior IOC_OPAL_ADD_USR_TO_LR > ioctl command. The command was extended to add user in ACE that > allows to read attributes listed above. > > Signed-off-by: Ondrej Kozina <okozina@redhat.com> > Tested-by: Luca Boccassi <bluca@debian.org> > Tested-by: Milan Broz <gmazyland@gmail.com> > --- > block/sed-opal.c | 140 ++++++++++++++++++++++++++++++++++ > include/linux/sed-opal.h | 1 + > include/uapi/linux/sed-opal.h | 11 +++ > 3 files changed, 152 insertions(+) > > diff --git a/block/sed-opal.c b/block/sed-opal.c > index 27abed4d60ef..9af2c72faf0e 100644 > --- a/block/sed-opal.c > +++ b/block/sed-opal.c > @@ -1445,6 +1445,116 @@ static int setup_locking_range(struct opal_dev *dev, void *data) > return finalize_and_send(dev, parse_and_check_status); > } > > +static int response_get_column(const struct parsed_resp *resp, int *iter, u8 column, u64 *value) > +{ > + const struct opal_resp_tok *tok; > + int n = *iter; > + u64 val; > + > + tok = response_get_token(resp, n); > + if (IS_ERR(tok)) > + return PTR_ERR(tok); > + > + 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. > + 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. :) > + > + return 0; > +} > + > +static int locking_range_status(struct opal_dev *dev, void *data) > +{ > + u8 lr_buffer[OPAL_UID_LENGTH]; > + u64 resp; > + bool rlocked, wlocked; > + int err, tok_n = 2; > + struct opal_lr_status *lrst = data; > + > + err = build_locking_range(lr_buffer, sizeof(lr_buffer), > + lrst->session.opal_key.lr); > + if (err) > + return err; > + > + err = generic_get_columns(dev, lr_buffer, OPAL_RANGESTART, OPAL_WRITELOCKED); > + if (err) { > + pr_debug("Couldn't get lr %u table columns %d to %d.\n", > + lrst->session.opal_key.lr, OPAL_RANGESTART, > + OPAL_WRITELOCKED); > + return err; > + } > + > + /* range start */ > + err = response_get_column(&dev->parsed, &tok_n, OPAL_RANGESTART, &lrst->range_start); > + if (err) > + return err; > + > + /* range length */ > + err = response_get_column(&dev->parsed, &tok_n, OPAL_RANGELENGTH, &lrst->range_length); > + if (err) > + return err; > + > + /* RLE */ > + err = response_get_column(&dev->parsed, &tok_n, OPAL_READLOCKENABLED, &resp); > + if (err) > + return err; > + > + lrst->RLE = !!resp; > + > + /* WLE */ > + err = response_get_column(&dev->parsed, &tok_n, OPAL_WRITELOCKENABLED, &resp); > + if (err) > + return err; > + > + lrst->WLE = !!resp; > + > + /* read locked */ > + err = response_get_column(&dev->parsed, &tok_n, OPAL_READLOCKED, &resp); > + if (err) > + return err; > + > + rlocked = !!resp; > + > + /* write locked */ > + err = response_get_column(&dev->parsed, &tok_n, OPAL_WRITELOCKED, &resp); > + if (err) > + return err; > + > + wlocked = !!resp; > + > + /*opal_lock_state can not map 'read locked' only state. */ > + lrst->l_state = OPAL_RW; > + if (rlocked && wlocked) > + lrst->l_state = OPAL_LK; > + else if (wlocked) > + lrst->l_state = OPAL_RO; > + else if (rlocked) { > + pr_debug("Can not report read locked only state.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int start_generic_opal_session(struct opal_dev *dev, > enum opal_uid auth, > enum opal_uid sp_type, > @@ -2617,6 +2727,33 @@ static int opal_setup_locking_range(struct opal_dev *dev, > return ret; > } > > +static int opal_locking_range_status(struct opal_dev *dev, > + struct opal_lr_status *opal_lrst, > + void __user *data) > +{ > + const struct opal_step lr_steps[] = { > + { start_auth_opal_session, &opal_lrst->session }, > + { locking_range_status, opal_lrst }, > + { end_opal_session, } > + }; > + int ret; > + > + mutex_lock(&dev->dev_lock); > + setup_opal_dev(dev); > + ret = execute_steps(dev, lr_steps, ARRAY_SIZE(lr_steps)); > + mutex_unlock(&dev->dev_lock); > + > + /* 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)) ? > + sizeof(*opal_lrst) - offsetof(struct opal_lr_status, range_start))) { > + pr_debug("Error copying status to userspace\n"); > + return -EFAULT; > + } > + > + return ret; > +} > + > static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw) > { > const struct opal_step pw_steps[] = { > @@ -2851,6 +2988,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) > case IOC_OPAL_GET_STATUS: > ret = opal_get_status(dev, arg); > break; > + case IOC_OPAL_GET_LR_STATUS: > + ret = opal_locking_range_status(dev, p, arg); > + break; > default: > break; > } > diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h > index 31ac562a17d7..042c1e2cb0ce 100644 > --- a/include/linux/sed-opal.h > +++ b/include/linux/sed-opal.h > @@ -45,6 +45,7 @@ static inline bool is_sed_ioctl(unsigned int cmd) > case IOC_OPAL_WRITE_SHADOW_MBR: > case IOC_OPAL_GENERIC_TABLE_RW: > case IOC_OPAL_GET_STATUS: > + case IOC_OPAL_GET_LR_STATUS: > return true; > } > return false; > 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. > + __u32 l_state; "locking_state"? > + __u8 align[4]; > +}; > + > struct opal_lock_unlock { > struct opal_session_info session; > __u32 l_state; > @@ -168,5 +178,6 @@ struct opal_status { > #define IOC_OPAL_WRITE_SHADOW_MBR _IOW('p', 234, struct opal_shadow_mbr) > #define IOC_OPAL_GENERIC_TABLE_RW _IOW('p', 235, struct opal_read_write_table) > #define IOC_OPAL_GET_STATUS _IOR('p', 236, struct opal_status) > +#define IOC_OPAL_GET_LR_STATUS _IOW('p', 237, struct opal_lr_status) > > #endif /* _UAPI_SED_OPAL_H */ > -- > 2.31.1 >
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.
On Wed, 5 Apr 2023 at 10:39, Ondrej Kozina <okozina@redhat.com> wrote: > > On 05. 04. 23 10:27, Christian Brauner wrote: > > On Wed, Mar 22, 2023 at 04:16:04PM +0100, Ondrej Kozina wrote: > >> 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. Yeah we really want to keep the parameters names the same across the various ioctl, otherwise it's going to get very confusing very quickly.
diff --git a/block/sed-opal.c b/block/sed-opal.c index 27abed4d60ef..9af2c72faf0e 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -1445,6 +1445,116 @@ static int setup_locking_range(struct opal_dev *dev, void *data) return finalize_and_send(dev, parse_and_check_status); } +static int response_get_column(const struct parsed_resp *resp, int *iter, u8 column, u64 *value) +{ + const struct opal_resp_tok *tok; + int n = *iter; + u64 val; + + tok = response_get_token(resp, n); + if (IS_ERR(tok)) + return PTR_ERR(tok); + + 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) { + 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; + + return 0; +} + +static int locking_range_status(struct opal_dev *dev, void *data) +{ + u8 lr_buffer[OPAL_UID_LENGTH]; + u64 resp; + bool rlocked, wlocked; + int err, tok_n = 2; + struct opal_lr_status *lrst = data; + + err = build_locking_range(lr_buffer, sizeof(lr_buffer), + lrst->session.opal_key.lr); + if (err) + return err; + + err = generic_get_columns(dev, lr_buffer, OPAL_RANGESTART, OPAL_WRITELOCKED); + if (err) { + pr_debug("Couldn't get lr %u table columns %d to %d.\n", + lrst->session.opal_key.lr, OPAL_RANGESTART, + OPAL_WRITELOCKED); + return err; + } + + /* range start */ + err = response_get_column(&dev->parsed, &tok_n, OPAL_RANGESTART, &lrst->range_start); + if (err) + return err; + + /* range length */ + err = response_get_column(&dev->parsed, &tok_n, OPAL_RANGELENGTH, &lrst->range_length); + if (err) + return err; + + /* RLE */ + err = response_get_column(&dev->parsed, &tok_n, OPAL_READLOCKENABLED, &resp); + if (err) + return err; + + lrst->RLE = !!resp; + + /* WLE */ + err = response_get_column(&dev->parsed, &tok_n, OPAL_WRITELOCKENABLED, &resp); + if (err) + return err; + + lrst->WLE = !!resp; + + /* read locked */ + err = response_get_column(&dev->parsed, &tok_n, OPAL_READLOCKED, &resp); + if (err) + return err; + + rlocked = !!resp; + + /* write locked */ + err = response_get_column(&dev->parsed, &tok_n, OPAL_WRITELOCKED, &resp); + if (err) + return err; + + wlocked = !!resp; + + /*opal_lock_state can not map 'read locked' only state. */ + lrst->l_state = OPAL_RW; + if (rlocked && wlocked) + lrst->l_state = OPAL_LK; + else if (wlocked) + lrst->l_state = OPAL_RO; + else if (rlocked) { + pr_debug("Can not report read locked only state.\n"); + return -EINVAL; + } + + return 0; +} + static int start_generic_opal_session(struct opal_dev *dev, enum opal_uid auth, enum opal_uid sp_type, @@ -2617,6 +2727,33 @@ static int opal_setup_locking_range(struct opal_dev *dev, return ret; } +static int opal_locking_range_status(struct opal_dev *dev, + struct opal_lr_status *opal_lrst, + void __user *data) +{ + const struct opal_step lr_steps[] = { + { start_auth_opal_session, &opal_lrst->session }, + { locking_range_status, opal_lrst }, + { end_opal_session, } + }; + int ret; + + mutex_lock(&dev->dev_lock); + setup_opal_dev(dev); + ret = execute_steps(dev, lr_steps, ARRAY_SIZE(lr_steps)); + mutex_unlock(&dev->dev_lock); + + /* 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), + sizeof(*opal_lrst) - offsetof(struct opal_lr_status, range_start))) { + pr_debug("Error copying status to userspace\n"); + return -EFAULT; + } + + return ret; +} + static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw) { const struct opal_step pw_steps[] = { @@ -2851,6 +2988,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) case IOC_OPAL_GET_STATUS: ret = opal_get_status(dev, arg); break; + case IOC_OPAL_GET_LR_STATUS: + ret = opal_locking_range_status(dev, p, arg); + break; default: break; } diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h index 31ac562a17d7..042c1e2cb0ce 100644 --- a/include/linux/sed-opal.h +++ b/include/linux/sed-opal.h @@ -45,6 +45,7 @@ static inline bool is_sed_ioctl(unsigned int cmd) case IOC_OPAL_WRITE_SHADOW_MBR: case IOC_OPAL_GENERIC_TABLE_RW: case IOC_OPAL_GET_STATUS: + case IOC_OPAL_GET_LR_STATUS: return true; } return false; 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 */ + __u32 l_state; + __u8 align[4]; +}; + struct opal_lock_unlock { struct opal_session_info session; __u32 l_state; @@ -168,5 +178,6 @@ struct opal_status { #define IOC_OPAL_WRITE_SHADOW_MBR _IOW('p', 234, struct opal_shadow_mbr) #define IOC_OPAL_GENERIC_TABLE_RW _IOW('p', 235, struct opal_read_write_table) #define IOC_OPAL_GET_STATUS _IOR('p', 236, struct opal_status) +#define IOC_OPAL_GET_LR_STATUS _IOW('p', 237, struct opal_lr_status) #endif /* _UAPI_SED_OPAL_H */