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

Christian Brauner
2023-04-05T08:28:03Z

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
>