[PATCH 0/4] block: add IOC_PR_READ_KEYS and IOC_PR_READ_RESERVATION ioctls

[PATCH 0/4] block: add IOC_PR_READ_KEYS and IOC_PR_READ_RESERVATION ioctls

[Stefan Hajnoczi]

This series exposes struct pr_ops pr_read_keys() and pr_read_reservations() to
userspace as ioctls, making it possible to list registered reservation keys and
report the current reservation on a block device.

The new ioctls are needed by applications or cluster managers that rely on
inspecting the PR state. This is something that has been possible with SCSI-
and NVME-specific commands but not with the PR ioctls. I hope to move QEMU from
SG_IO to PR ioctls so that NVMe host block devices can be supported alongside
SCSI devices without protocol-specific commands.

These ioctls will also make troubleshooting possible with the blkpr(8)
util-linux tool, for which I have prepared a separate patch series.

Stefan Hajnoczi (4):
  scsi: sd: reject invalid pr_read_keys() num_keys values
  nvme: reject invalid pr_read_keys() num_keys values
  block: add IOC_PR_READ_KEYS ioctl
  block: add IOC_PR_READ_RESERVATION ioctl

 include/uapi/linux/pr.h | 14 ++++++++
 block/ioctl.c           | 79 +++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/pr.c  |  5 +++
 drivers/scsi/sd.c       | 11 +++++-
 4 files changed, 108 insertions(+), 1 deletion(-)

-- 
2.52.0

[PATCH 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

The pr_read_keys() interface has a u32 num_keys parameter. The SCSI
PERSISTENT RESERVE IN command has a maximum READ KEYS service action
size of 65536 bytes. Reject num_keys values that are too large to fit
into the SCSI command.

This will become important when pr_read_keys() is exposed to untrusted
userspace via an <linux/pr.h> ioctl.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/scsi/sd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 0252d3f6bed17..d65646f35f453 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1974,9 +1974,18 @@ static int sd_pr_read_keys(struct block_device *bdev, struct pr_keys *keys_info)
 {
 	int result, i, data_offset, num_copy_keys;
 	u32 num_keys = keys_info->num_keys;
-	int data_len = num_keys * 8 + 8;
+	int data_len;
 	u8 *data;
 
+	/*
+	 * Each reservation key takes 8 bytes and there is an 8-byte header
+	 * before the reservation key list. The total size must fit into the
+	 * 16-bit ALLOCATION LENGTH field.
+	 */
+	if (num_keys > (65536 - 8) / 8)
+		return -EINVAL;
+
+	data_len = num_keys * 8 + 8;
 	data = kzalloc(data_len, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.52.0

Re: [PATCH 1/4] scsi: sd: reject invalid pr_read_keys() num_keys values

[Hannes Reinecke]

On 11/26/25 17:35, Stefan Hajnoczi wrote:
> The pr_read_keys() interface has a u32 num_keys parameter. The SCSI
> PERSISTENT RESERVE IN command has a maximum READ KEYS service action
> size of 65536 bytes. Reject num_keys values that are too large to fit
> into the SCSI command.
> 
> This will become important when pr_read_keys() is exposed to untrusted
> userspace via an <linux/pr.h> ioctl.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   drivers/scsi/sd.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 0252d3f6bed17..d65646f35f453 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1974,9 +1974,18 @@ static int sd_pr_read_keys(struct block_device *bdev, struct pr_keys *keys_info)
>   {
>   	int result, i, data_offset, num_copy_keys;
>   	u32 num_keys = keys_info->num_keys;
> -	int data_len = num_keys * 8 + 8;
> +	int data_len;
>   	u8 *data;
>   
> +	/*
> +	 * Each reservation key takes 8 bytes and there is an 8-byte header
> +	 * before the reservation key list. The total size must fit into the
> +	 * 16-bit ALLOCATION LENGTH field.
> +	 */
> +	if (num_keys > (65536 - 8) / 8)

Odd coding. Maybe (USHRT_MAX / 8) - 1 ?

> +		return -EINVAL;
> +
> +	data_len = num_keys * 8 + 8;
>   	data = kzalloc(data_len, GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Stefan Hajnoczi]

The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
Reservation Report command has a u32 maximum length. Reject num_keys
values that are too large to fit.

This will become important when pr_read_keys() is exposed to untrusted
userspace via an <linux/pr.h> ioctl.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/nvme/host/pr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index ca6a74607b139..476a0518a11ca 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -233,6 +233,11 @@ static int nvme_pr_read_keys(struct block_device *bdev,
 	int ret, i;
 	bool eds;
 
+	/* Check that keys fit into u32 rse_len */
+	if (num_keys > -(u32)offsetof(typeof(*rse), regctl_eds) /
+	               sizeof(rse->regctl_eds[0]))
+		return -EINVAL;
+
 	/*
 	 * Assume we are using 128-bit host IDs and allocate a buffer large
 	 * enough to get enough keys to fill the return keys buffer.
-- 
2.52.0

Re: [PATCH 2/4] nvme: reject invalid pr_read_keys() num_keys values

[Hannes Reinecke]

On 11/26/25 17:35, Stefan Hajnoczi wrote:
> The pr_read_keys() interface has a u32 num_keys parameter. The NVMe
> Reservation Report command has a u32 maximum length. Reject num_keys
> values that are too large to fit.
> 
> This will become important when pr_read_keys() is exposed to untrusted
> userspace via an <linux/pr.h> ioctl.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   drivers/nvme/host/pr.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> index ca6a74607b139..476a0518a11ca 100644
> --- a/drivers/nvme/host/pr.c
> +++ b/drivers/nvme/host/pr.c
> @@ -233,6 +233,11 @@ static int nvme_pr_read_keys(struct block_device *bdev,
>   	int ret, i;
>   	bool eds;
>   
> +	/* Check that keys fit into u32 rse_len */
> +	if (num_keys > -(u32)offsetof(typeof(*rse), regctl_eds) /
> +	               sizeof(rse->regctl_eds[0]))
> +		return -EINVAL;
> +
>   	/*
>   	 * Assume we are using 128-bit host IDs and allocate a buffer large
>   	 * enough to get enough keys to fill the return keys buffer.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Stefan Hajnoczi]

Add a Persistent Reservations ioctl to read the list of currently
registered reservation keys. This calls the pr_ops->read_keys() function
that was previously added in commit c787f1baa503 ("block: Add PR
callouts for read keys and reservation") but was only used by the
in-kernel SCSI target so far.

The IOC_PR_READ_KEYS ioctl is necessary so that userspace applications
that rely on Persistent Reservations ioctls have a way of inspecting the
current state. Cluster managers and validation tests need this
functionality.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/uapi/linux/pr.h |  7 ++++++
 block/ioctl.c           | 51 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index d8126415966f3..fcb74eab92c80 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -56,6 +56,12 @@ struct pr_clear {
 	__u32	__pad;
 };
 
+struct pr_read_keys {
+	__u32	generation;
+	__u32	num_keys;
+	__u64	keys_ptr;
+};
+
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
@@ -64,5 +70,6 @@ struct pr_clear {
 #define IOC_PR_PREEMPT		_IOW('p', 203, struct pr_preempt)
 #define IOC_PR_PREEMPT_ABORT	_IOW('p', 204, struct pr_preempt)
 #define IOC_PR_CLEAR		_IOW('p', 205, struct pr_clear)
+#define IOC_PR_READ_KEYS	_IOWR('p', 206, struct pr_read_keys)
 
 #endif /* _UAPI_PR_H */
diff --git a/block/ioctl.c b/block/ioctl.c
index d7489a56b33c3..e87c424c15ae9 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/capability.h>
+#include <linux/cleanup.h>
 #include <linux/compat.h>
 #include <linux/blkdev.h>
 #include <linux/export.h>
@@ -423,6 +424,54 @@ static int blkdev_pr_clear(struct block_device *bdev, blk_mode_t mode,
 	return ops->pr_clear(bdev, c.key);
 }
 
+static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
+		struct pr_read_keys __user *arg)
+{
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	struct pr_keys *keys_info __free(kfree) = NULL;
+	struct pr_read_keys inout;
+	int ret;
+
+	if (!blkdev_pr_allowed(bdev, mode))
+		return -EPERM;
+	if (!ops || !ops->pr_read_keys)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&inout, arg, sizeof(inout)))
+		return -EFAULT;
+
+	if (inout.num_keys > -sizeof(*keys_info) / sizeof(keys_info->keys[0]))
+		return -EINVAL;
+
+	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
+
+	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
+	if (!keys_info)
+		return -ENOMEM;
+
+	keys_info->num_keys = inout.num_keys;
+
+	ret = ops->pr_read_keys(bdev, keys_info);
+	if (ret)
+		return ret;
+
+	/* Copy out individual keys */
+	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
+	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
+	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
+
+	if (copy_to_user(keys_ptr, keys_info->keys, keys_copy_len))
+		return -EFAULT;
+
+	/* Copy out the arg struct */
+	inout.generation = keys_info->generation;
+	inout.num_keys = keys_info->num_keys;
+
+	if (copy_to_user(arg, &inout, sizeof(inout)))
+		return -EFAULT;
+	return ret;
+}
+
 static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
 		unsigned long arg)
 {
@@ -644,6 +693,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
 		return blkdev_pr_preempt(bdev, mode, argp, true);
 	case IOC_PR_CLEAR:
 		return blkdev_pr_clear(bdev, mode, argp);
+	case IOC_PR_READ_KEYS:
+		return blkdev_pr_read_keys(bdev, mode, argp);
 	default:
 		return blk_get_meta_cap(bdev, cmd, argp);
 	}
-- 
2.52.0

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[kernel test robot]

Hi Stefan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on axboe/for-next jejb-scsi/for-next linus/master v6.18-rc7 next-20251126]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Hajnoczi/scsi-sd-reject-invalid-pr_read_keys-num_keys-values/20251127-003756
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20251126163600.583036-4-stefanha%40redhat.com
patch subject: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl
config: loongarch-allnoconfig (https://download.01.org/0day-ci/archive/20251127/202511270125.CJ6M2RHv-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 9e9fe08b16ea2c4d9867fb4974edf2a3776d6ece)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251127/202511270125.CJ6M2RHv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511270125.CJ6M2RHv-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> block/ioctl.c:443:21: warning: result of comparison of constant 2305843009213693951 with expression of type '__u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
     443 |         if (inout.num_keys > -sizeof(*keys_info) / sizeof(keys_info->keys[0]))
         |             ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +443 block/ioctl.c

   426	
   427	static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
   428			struct pr_read_keys __user *arg)
   429	{
   430		const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
   431		struct pr_keys *keys_info __free(kfree) = NULL;
   432		struct pr_read_keys inout;
   433		int ret;
   434	
   435		if (!blkdev_pr_allowed(bdev, mode))
   436			return -EPERM;
   437		if (!ops || !ops->pr_read_keys)
   438			return -EOPNOTSUPP;
   439	
   440		if (copy_from_user(&inout, arg, sizeof(inout)))
   441			return -EFAULT;
   442	
 > 443		if (inout.num_keys > -sizeof(*keys_info) / sizeof(keys_info->keys[0]))
   444			return -EINVAL;
   445	
   446		size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
   447	
   448		keys_info = kzalloc(keys_info_len, GFP_KERNEL);
   449		if (!keys_info)
   450			return -ENOMEM;
   451	
   452		keys_info->num_keys = inout.num_keys;
   453	
   454		ret = ops->pr_read_keys(bdev, keys_info);
   455		if (ret)
   456			return ret;
   457	
   458		/* Copy out individual keys */
   459		u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
   460		u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
   461		size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
   462	
   463		if (copy_to_user(keys_ptr, keys_info->keys, keys_copy_len))
   464			return -EFAULT;
   465	
   466		/* Copy out the arg struct */
   467		inout.generation = keys_info->generation;
   468		inout.num_keys = keys_info->num_keys;
   469	
   470		if (copy_to_user(arg, &inout, sizeof(inout)))
   471			return -EFAULT;
   472		return ret;
   473	}
   474	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Hannes Reinecke]

On 11/26/25 17:35, Stefan Hajnoczi wrote:
> Add a Persistent Reservations ioctl to read the list of currently
> registered reservation keys. This calls the pr_ops->read_keys() function
> that was previously added in commit c787f1baa503 ("block: Add PR
> callouts for read keys and reservation") but was only used by the
> in-kernel SCSI target so far.
> 
> The IOC_PR_READ_KEYS ioctl is necessary so that userspace applications
> that rely on Persistent Reservations ioctls have a way of inspecting the
> current state. Cluster managers and validation tests need this
> functionality.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/uapi/linux/pr.h |  7 ++++++
>   block/ioctl.c           | 51 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 58 insertions(+)
> 
> diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
> index d8126415966f3..fcb74eab92c80 100644
> --- a/include/uapi/linux/pr.h
> +++ b/include/uapi/linux/pr.h
> @@ -56,6 +56,12 @@ struct pr_clear {
>   	__u32	__pad;
>   };
>   
> +struct pr_read_keys {
> +	__u32	generation;
> +	__u32	num_keys;
> +	__u64	keys_ptr;
> +};
> +
>   #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
>   
>   #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
> @@ -64,5 +70,6 @@ struct pr_clear {
>   #define IOC_PR_PREEMPT		_IOW('p', 203, struct pr_preempt)
>   #define IOC_PR_PREEMPT_ABORT	_IOW('p', 204, struct pr_preempt)
>   #define IOC_PR_CLEAR		_IOW('p', 205, struct pr_clear)
> +#define IOC_PR_READ_KEYS	_IOWR('p', 206, struct pr_read_keys)
>   
>   #endif /* _UAPI_PR_H */
> diff --git a/block/ioctl.c b/block/ioctl.c
> index d7489a56b33c3..e87c424c15ae9 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   #include <linux/capability.h>
> +#include <linux/cleanup.h>
>   #include <linux/compat.h>
>   #include <linux/blkdev.h>
>   #include <linux/export.h>
> @@ -423,6 +424,54 @@ static int blkdev_pr_clear(struct block_device *bdev, blk_mode_t mode,
>   	return ops->pr_clear(bdev, c.key);
>   }
>   
> +static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
> +		struct pr_read_keys __user *arg)
> +{
> +	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
> +	struct pr_keys *keys_info __free(kfree) = NULL;
> +	struct pr_read_keys inout;
> +	int ret;
> +
> +	if (!blkdev_pr_allowed(bdev, mode))
> +		return -EPERM;
> +	if (!ops || !ops->pr_read_keys)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&inout, arg, sizeof(inout)))
> +		return -EFAULT;
> +
> +	if (inout.num_keys > -sizeof(*keys_info) / sizeof(keys_info->keys[0]))
> +		return -EINVAL;
> +

0-sizeof()? What's that supposed to achieve? Plus inout.numkeys is 
unsigned (as the kbuild robot indicated).

> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
> +
> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
> +	if (!keys_info)
> +		return -ENOMEM;
> +
> +	keys_info->num_keys = inout.num_keys;
> +
> +	ret = ops->pr_read_keys(bdev, keys_info);
> +	if (ret)
> +		return ret;
> +
> +	/* Copy out individual keys */
> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);

We just had the discussion about variable declarations on the ksummit 
lists; I really would prefer to have all declarations at the start of 
the scope (read: at the start of the function here).

> +
> +	if (copy_to_user(keys_ptr, keys_info->keys, keys_copy_len))
> +		return -EFAULT;
> +
> +	/* Copy out the arg struct */
> +	inout.generation = keys_info->generation;
> +	inout.num_keys = keys_info->num_keys;
> +
> +	if (copy_to_user(arg, &inout, sizeof(inout)))
> +		return -EFAULT;
> +	return ret;
> +}
> +
>   static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
>   		unsigned long arg)
>   {
> @@ -644,6 +693,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
>   		return blkdev_pr_preempt(bdev, mode, argp, true);
>   	case IOC_PR_CLEAR:
>   		return blkdev_pr_clear(bdev, mode, argp);
> +	case IOC_PR_READ_KEYS:
> +		return blkdev_pr_read_keys(bdev, mode, argp);
>   	default:
>   		return blk_get_meta_cap(bdev, cmd, argp);
>   	Cheers,Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Krzysztof Kozlowski]

On 27/11/2025 08:07, Hannes Reinecke wrote:
> 
>> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
>> +
>> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
>> +	if (!keys_info)
>> +		return -ENOMEM;
>> +
>> +	keys_info->num_keys = inout.num_keys;
>> +
>> +	ret = ops->pr_read_keys(bdev, keys_info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Copy out individual keys */
>> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
>> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
>> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
> 
> We just had the discussion about variable declarations on the ksummit 
> lists; I really would prefer to have all declarations at the start of 
> the scope (read: at the start of the function here).

Then also cleanup.h should not be used here.

Best regards,
Krzysztof

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
> On 27/11/2025 08:07, Hannes Reinecke wrote:
> > 
> >> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
> >> +
> >> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
> >> +	if (!keys_info)
> >> +		return -ENOMEM;
> >> +
> >> +	keys_info->num_keys = inout.num_keys;
> >> +
> >> +	ret = ops->pr_read_keys(bdev, keys_info);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Copy out individual keys */
> >> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
> >> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
> >> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
> > 
> > We just had the discussion about variable declarations on the ksummit 
> > lists; I really would prefer to have all declarations at the start of 
> > the scope (read: at the start of the function here).
> 
> Then also cleanup.h should not be used here.

Hi Krzysztof,
The documentation in cleanup.h says:

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
       ^^^^^^^^^^^^^^
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

This is a recommendation, not mandatory. It is also describing a
scenario that does not apply here.

There are many examples of existing users of __free() initialized to
NULL:

  $ git grep '__free(' | grep ' = NULL' | wc -l
  491

To me it seems like it is okay to use cleanup.h in this fashion. Did I
miss something?

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Krzysztof Kozlowski]

On 01/12/2025 16:06, Stefan Hajnoczi wrote:
> On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
>> On 27/11/2025 08:07, Hannes Reinecke wrote:
>>>
>>>> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
>>>> +
>>>> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
>>>> +	if (!keys_info)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	keys_info->num_keys = inout.num_keys;
>>>> +
>>>> +	ret = ops->pr_read_keys(bdev, keys_info);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Copy out individual keys */
>>>> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
>>>> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
>>>> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
>>>
>>> We just had the discussion about variable declarations on the ksummit 
>>> lists; I really would prefer to have all declarations at the start of 
>>> the scope (read: at the start of the function here).
>>
>> Then also cleanup.h should not be used here.
> 
> Hi Krzysztof,
> The documentation in cleanup.h says:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>        ^^^^^^^^^^^^^^
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 
> This is a recommendation, not mandatory. It is also describing a
> scenario that does not apply here.

If you have actual argument, so allocation in some if branch, the of course.

> 
> There are many examples of existing users of __free() initialized to
> NULL:
> 
>   $ git grep '__free(' | grep ' = NULL' | wc -l
>   491
> 

There is tons of incorrect usage, some I started already fixing.
Maintainers were changing when applying the correct code (correct patch)
into incorrect declaration without constructor and separate assignment.

Then contributors started adding patches at least making NULL
assignment, but still not following recommendation.

Then contributors started adding patches mixing cleanup.h with gotos,
also clearly discouraged.

Sorry, but please never use that argument. People did not read cleanup.h
and produced poor code. Maintainers did not read cleanup.h and changed
correct code into poor code.

Existing poor code, just its existence, is never the actual argument in
discussion.

> To me it seems like it is okay to use cleanup.h in this fashion. Did I
> miss something?



Best regards,
Krzysztof

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On Mon, Dec 01, 2025 at 05:26:27PM +0100, Krzysztof Kozlowski wrote:
> On 01/12/2025 16:06, Stefan Hajnoczi wrote:
> > On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
> >> On 27/11/2025 08:07, Hannes Reinecke wrote:
> >>>
> >>>> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
> >>>> +
> >>>> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
> >>>> +	if (!keys_info)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	keys_info->num_keys = inout.num_keys;
> >>>> +
> >>>> +	ret = ops->pr_read_keys(bdev, keys_info);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	/* Copy out individual keys */
> >>>> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
> >>>> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
> >>>> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
> >>>
> >>> We just had the discussion about variable declarations on the ksummit 
> >>> lists; I really would prefer to have all declarations at the start of 
> >>> the scope (read: at the start of the function here).
> >>
> >> Then also cleanup.h should not be used here.
> > 
> > Hi Krzysztof,
> > The documentation in cleanup.h says:
> > 
> >  * Given that the "__free(...) = NULL" pattern for variables defined at
> >  * the top of the function poses this potential interdependency problem
> >  * the recommendation is to always define and assign variables in one
> >        ^^^^^^^^^^^^^^
> >  * statement and not group variable definitions at the top of the
> >  * function when __free() is used.
> > 
> > This is a recommendation, not mandatory. It is also describing a
> > scenario that does not apply here.
> 
> If you have actual argument, so allocation in some if branch, the of course.

I'm pointing out that the documentation uses the word "recommendation",
which is usually not considered mandatory but a suggestion.

Please update the documentation to clarify that __free() _must_ be
assigned the real value (no NULL initialization) so that it's clear this
is not a suggestion but mandatory.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
> On 27/11/2025 08:07, Hannes Reinecke wrote:
> > 
> >> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
> >> +
> >> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
> >> +	if (!keys_info)
> >> +		return -ENOMEM;
> >> +
> >> +	keys_info->num_keys = inout.num_keys;
> >> +
> >> +	ret = ops->pr_read_keys(bdev, keys_info);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Copy out individual keys */
> >> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
> >> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
> >> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
> > 
> > We just had the discussion about variable declarations on the ksummit 
> > lists; I really would prefer to have all declarations at the start of 
> > the scope (read: at the start of the function here).
> 
> Then also cleanup.h should not be used here.

Hi Krzysztof,
Christoph Hellwig replied to the v2 series, also against using __free().
Regardless of the reply I just sent to you about whether cleanup.h may
or may not be used in code that forbids declarations midway through a
scope, I will be dropping it in v3.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Krzysztof Kozlowski]

On 01/12/2025 16:14, Stefan Hajnoczi wrote:
> On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
>> On 27/11/2025 08:07, Hannes Reinecke wrote:
>>>
>>>> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
>>>> +
>>>> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
>>>> +	if (!keys_info)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	keys_info->num_keys = inout.num_keys;
>>>> +
>>>> +	ret = ops->pr_read_keys(bdev, keys_info);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Copy out individual keys */
>>>> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
>>>> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
>>>> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
>>>
>>> We just had the discussion about variable declarations on the ksummit 
>>> lists; I really would prefer to have all declarations at the start of 
>>> the scope (read: at the start of the function here).
>>
>> Then also cleanup.h should not be used here.
> 
> Hi Krzysztof,
> Christoph Hellwig replied to the v2 series, also against using __free().

That's perfectly fine to dislike cleanup.h. It's fair. What is not fine
is using it against its recommendations. Either you take entire
cleanup.h with its oddities or don't use it.

> Regardless of the reply I just sent to you about whether cleanup.h may
> or may not be used in code that forbids declarations midway through a
> scope, I will be dropping it in v3.
> 

Best regards,
Krzysztof

Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

[Krzysztof Kozlowski]

On 26/11/2025 17:35, Stefan Hajnoczi wrote:
> +
>  #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
>  
>  #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
> @@ -64,5 +70,6 @@ struct pr_clear {
>  #define IOC_PR_PREEMPT		_IOW('p', 203, struct pr_preempt)
>  #define IOC_PR_PREEMPT_ABORT	_IOW('p', 204, struct pr_preempt)
>  #define IOC_PR_CLEAR		_IOW('p', 205, struct pr_clear)
> +#define IOC_PR_READ_KEYS	_IOWR('p', 206, struct pr_read_keys)
>  
>  #endif /* _UAPI_PR_H */
> diff --git a/block/ioctl.c b/block/ioctl.c
> index d7489a56b33c3..e87c424c15ae9 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/capability.h>
> +#include <linux/cleanup.h>
>  #include <linux/compat.h>
>  #include <linux/blkdev.h>
>  #include <linux/export.h>
> @@ -423,6 +424,54 @@ static int blkdev_pr_clear(struct block_device *bdev, blk_mode_t mode,
>  	return ops->pr_clear(bdev, c.key);
>  }
>  
> +static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
> +		struct pr_read_keys __user *arg)
> +{
> +	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
> +	struct pr_keys *keys_info __free(kfree) = NULL;


This is an undesired syntax explicitly documented as one to avoid. You
need here proper assignment, not NULL. Please don't use cleanup.h if you
do not intend to follow it because it does not make the code simpler.



Best regards,
Krzysztof

[PATCH 4/4] block: add IOC_PR_READ_RESERVATION ioctl

[Stefan Hajnoczi]

Add a Persistent Reservations ioctl to read the current reservation.
This calls the pr_ops->read_reservation() function that was previously
added in commit c787f1baa503 ("block: Add PR callouts for read keys and
reservation") but was only used by the in-kernel SCSI target so far.

The IOC_PR_READ_RESERVATION ioctl is necessary so that userspace
applications that rely on Persistent Reservations ioctls have a way of
inspecting the current state. Cluster managers and validation tests need
this functionality.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/uapi/linux/pr.h |  7 +++++++
 block/ioctl.c           | 28 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index fcb74eab92c80..847f3051057af 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -62,6 +62,12 @@ struct pr_read_keys {
 	__u64	keys_ptr;
 };
 
+struct pr_read_reservation {
+	__u64	key;
+	__u32	generation;
+	__u32	type;
+};
+
 #define PR_FL_IGNORE_KEY	(1 << 0)	/* ignore existing key */
 
 #define IOC_PR_REGISTER		_IOW('p', 200, struct pr_registration)
@@ -71,5 +77,6 @@ struct pr_read_keys {
 #define IOC_PR_PREEMPT_ABORT	_IOW('p', 204, struct pr_preempt)
 #define IOC_PR_CLEAR		_IOW('p', 205, struct pr_clear)
 #define IOC_PR_READ_KEYS	_IOWR('p', 206, struct pr_read_keys)
+#define IOC_PR_READ_RESERVATION	_IOR('p', 207, struct pr_read_reservation)
 
 #endif /* _UAPI_PR_H */
diff --git a/block/ioctl.c b/block/ioctl.c
index e87c424c15ae9..7e6d5e532d16b 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -472,6 +472,32 @@ static int blkdev_pr_read_keys(struct block_device *bdev, blk_mode_t mode,
 	return ret;
 }
 
+static int blkdev_pr_read_reservation(struct block_device *bdev,
+		blk_mode_t mode, struct pr_read_reservation __user *arg)
+{
+	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+	struct pr_held_reservation rsv = {};
+	struct pr_read_reservation out = {};
+	int ret;
+
+	if (!blkdev_pr_allowed(bdev, mode))
+		return -EPERM;
+	if (!ops || !ops->pr_read_reservation)
+		return -EOPNOTSUPP;
+
+	ret = ops->pr_read_reservation(bdev, &rsv);
+	if (ret)
+		return ret;
+
+	out.key = rsv.key;
+	out.generation = rsv.generation;
+	out.type = rsv.type;
+
+	if (copy_to_user(arg, &out, sizeof(out)))
+		return -EFAULT;
+	return 0;
+}
+
 static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
 		unsigned long arg)
 {
@@ -695,6 +721,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
 		return blkdev_pr_clear(bdev, mode, argp);
 	case IOC_PR_READ_KEYS:
 		return blkdev_pr_read_keys(bdev, mode, argp);
+	case IOC_PR_READ_RESERVATION:
+		return blkdev_pr_read_reservation(bdev, mode, argp);
 	default:
 		return blk_get_meta_cap(bdev, cmd, argp);
 	}
-- 
2.52.0

Re: [PATCH 4/4] block: add IOC_PR_READ_RESERVATION ioctl

[Hannes Reinecke]

On 11/26/25 17:36, Stefan Hajnoczi wrote:
> Add a Persistent Reservations ioctl to read the current reservation.
> This calls the pr_ops->read_reservation() function that was previously
> added in commit c787f1baa503 ("block: Add PR callouts for read keys and
> reservation") but was only used by the in-kernel SCSI target so far.
> 
> The IOC_PR_READ_RESERVATION ioctl is necessary so that userspace
> applications that rely on Persistent Reservations ioctls have a way of
> inspecting the current state. Cluster managers and validation tests need
> this functionality.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/uapi/linux/pr.h |  7 +++++++
>   block/ioctl.c           | 28 ++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich