SG_IO ioctl regression

From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Keith Busch <kbusch@kernel.org>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	Douglas Gilbert <dgilbert@interlog.com>,
	Damien Le Moal <damien.lemoal@opensource.wdc.com>,
	Jens Axboe <axboe@kernel.dk>
Subject: SG_IO ioctl regression
Date: Wed, 11 Jan 2023 14:38:56 +0000	[thread overview]
Message-ID: <Y77J/w0gf2nIDMd/@x1-carbon> (raw)
In-Reply-To: <20230105190741.2405013-6-kbuschmeta!com>

On Thu, Jan 05, 2023 at 07:07:34PM +0000, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> This is more efficient that iter_iov.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  block/blk-map.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-map.c b/block/blk-map.c
> index 19940c978c73b..4cf83eae9f2e8 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -641,7 +641,7 @@ int blk_rq_map_user_iov(struct request_queue *q, stru=
> ct request *rq,
>		copy =3D true;
>	else if (iov_iter_is_bvec(iter))
>		map_bvec =3D true;
> -	else if (!iter_is_iovec(iter))
> +	else if (!user_backed_iter(iter))
>		copy =3D true;
>	else if (queue_virt_boundary(q))
>		copy =3D queue_virt_boundary(q) & iov_iter_gap_alignment(iter);
> @@ -682,9 +682,8 @@ int blk_rq_map_user(struct request_queue *q, struct r=
> equest *rq,
>		    struct rq_map_data *map_data, void __user *ubuf,
>		    unsigned long len, gfp_t gfp_mask)
>  {
> -	struct iovec iov;
>	struct iov_iter i;
> -	int ret =3D import_single_range(rq_data_dir(rq), ubuf, len, &iov, &i);
> +	int ret =3D import_ubuf(rq_data_dir(rq), ubuf, len, &i);
> =20
>	if (unlikely(ret < 0))
>		return ret;
> --=20
> 2.30.2
>
>
> --

Hello Keith,

It appears that this commit breaks SG_IO ioctl.

A git bisect returned the following commit:

commit a696647a3b58c7a2dddd6eabfc824be826613211
Author: Keith Busch <kbusch@kernel.org>
Date:   Thu Jan 5 11:07:34 2023 -0800

    block: use iter_ubuf for single range

    This is more efficient than iter_iov.

    Signed-off-by: Keith Busch <kbusch@kernel.org>
    Signed-off-by: Jens Axboe <axboe@kernel.dk>
    Reviewed-by: Christoph Hellwig <hch@lst.de>

 block/blk-map.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

When using linux-next tag next-20230111 :

$ sudo sg_inq /dev/sg0
sg_inq failed: Bad address

If I simply revert your commit
a696647a3b58 ("block: use iter_ubuf for single range"),
things start working again:

$ sudo sg_inq /dev/sg0
standard INQUIRY:
  PQual=0  PDT=0  RMB=0  LU_CONG=0  hot_pluggable=0  version=0x05  [SPC-3]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
  SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
  EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  [Linked=0]  [TranDis=0]  CmdQue=1
  [SPI: Clocking=0x0  QAS=0  IUS=0]
    length=96 (0x60)   Peripheral device type: disk
 Vendor identification: ATA
 Product identification: QEMU HARDDISK
 Product revision level: 2.5+
 Unit serial number: QM00005

I'm quite surprised that we don't have any automated tests that
have caught/reported this already, as SG is quite heavily used.

Kind regards,
Niklas