Re: [PATCH v3 12/19] block,nvme,scsi,dm: Add blk_status to pr_ops callouts

Christoph Hellwig
2022-11-01T10:15:29Z

On Sun, Oct 30, 2022 at 06:05:35PM -0500, Mike Christie wrote:
> The problem I hit is that in the ioctl code I then have to do:
>
> @@ -269,7 +270,14 @@ static int blkdev_pr_register(struct block_device *bdev,
>
> if (reg.flags & ~PR_FL_IGNORE_KEY)
> return -EOPNOTSUPP;
> - return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
> + ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
> + if (ret == -EBADE) {
> + if (bdev_is_nvme(bdev))
> + ret = NVME_SC_RESERVATION_CONFLICT;
> + else if (bdev_is_scsi(bdev)
> + ret = SAM_STAT_RESERVATION_CONFLICT;
> + }
> + return ret;

Eww. We should have never leaked protocol specific values out to
userspace. This is an original bug I introduceѕ, so all blame is on me.

I suspect the right way to fix is is to keep the numeric value of
SAM_STAT_RESERVATION_CONFLICT and give it a new constant exposed in
the uapi header, assuming that SCSI is the thing people actually
used the PR API for, and nvme was just an nice little add-on.

Now if an errno value or blk_status_t is returned from the method
should not matter for this fix, but in the long run I think the
blk_status_t would be cleaner than the int used for errno, and
that will also prevent us from returning accidental non-intended
values.

Btw, I also thing we should rename BLK_STS_NEXUS to
BLK_STS_RESERVATION_CONFLICT (assuming s390 is ok with that), as that
has much better documentary value.

> + case BLK_STS_TRANSPORT:
> + if (bdev_is_nvme(bdev))
> + ret = NVME_SC_HOST_PATH_ERROR;
> + else if (bdev_is_scsi(bdev)
> + ret = DID_TRANSPORT_FAILFAST or DID_TRANSPORT_MARGINAL;
> + break;

And we'll need an uapi value for this as well.

> + case BLK_STS_NOTSUPP:

and maybe this unless we can just get away with the negative errno
value.