Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Jens Axboe <axboe@kernel.dk>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Douglas Gilbert <dgilbert@interlog.com>,
	linux-kernel@vger.kernel.org, y2038@lists.linaro.org,
	Steffen Maier <maier@linux.ibm.com>,
	linux-scsi@vger.kernel.org,
	Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-block@vger.kernel.org
Subject: Re: [PATCH 02/24] compat: scsi: sg: fix v3 compat read/write interface
Date: Thu, 12 Dec 2019 08:25:06 -0800
Message-ID: <20191212162506.GA27991@infradead.org> (raw)
In-Reply-To: <20191211204306.1207817-3-arnd@arndb.de>

On Wed, Dec 11, 2019 at 09:42:36PM +0100, Arnd Bergmann wrote:
> In the v5.4 merge window, a cleanup patch from Al Viro conflicted
> with my rework of the compat handling for sg.c read(). Linus Torvalds
> did a correct merge but pointed out that the resulting code is still
> unsatisfactory.
> 
> I later noticed that the sg_new_read() function still gets the compat
> mode wrong, when the 'count' argument is large enough to pass a
> compat_sg_io_hdr object, but not a nativ sg_io_hdr.
> 
> To address both of these, move the definition of compat_sg_io_hdr
> into a scsi/sg.h to make it visible to sg.c and rewrite the logic
> for reading req_pack_id as well as the size check to a simpler
> version that gets the expected results.
> 
> Fixes: c35a5cfb4150 ("scsi: sg: sg_read(): simplify reading ->pack_id of userland sg_io_hdr_t")
> Fixes: 98aaaec4a150 ("compat_ioctl: reimplement SG_IO handling")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  block/scsi_ioctl.c |  29 +----------
>  drivers/scsi/sg.c  | 125 +++++++++++++++++++++------------------------
>  include/scsi/sg.h  |  30 +++++++++++
>  3 files changed, 89 insertions(+), 95 deletions(-)
> 
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 650bade5ea5a..b61dbf4d8443 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -20,6 +20,7 @@
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_ioctl.h>
>  #include <scsi/scsi_cmnd.h>
> +#include <scsi/sg.h>
>  
>  struct blk_cmd_filter {
>  	unsigned long read_ok[BLK_SCSI_CMD_PER_LONG];
> @@ -550,34 +551,6 @@ static inline int blk_send_start_stop(struct request_queue *q,
>  	return __blk_send_generic(q, bd_disk, GPCMD_START_STOP_UNIT, data);
>  }
>  
> -#ifdef CONFIG_COMPAT
> -struct compat_sg_io_hdr {
> -	compat_int_t interface_id;	/* [i] 'S' for SCSI generic (required) */
> -	compat_int_t dxfer_direction;	/* [i] data transfer direction  */
> -	unsigned char cmd_len;		/* [i] SCSI command length ( <= 16 bytes) */
> -	unsigned char mx_sb_len;	/* [i] max length to write to sbp */
> -	unsigned short iovec_count;	/* [i] 0 implies no scatter gather */
> -	compat_uint_t dxfer_len;	/* [i] byte count of data transfer */
> -	compat_uint_t dxferp;		/* [i], [*io] points to data transfer memory
> -						or scatter gather list */
> -	compat_uptr_t cmdp;		/* [i], [*i] points to command to perform */
> -	compat_uptr_t sbp;		/* [i], [*o] points to sense_buffer memory */
> -	compat_uint_t timeout;		/* [i] MAX_UINT->no timeout (unit: millisec) */
> -	compat_uint_t flags;		/* [i] 0 -> default, see SG_FLAG... */
> -	compat_int_t pack_id;		/* [i->o] unused internally (normally) */
> -	compat_uptr_t usr_ptr;		/* [i->o] unused internally */
> -	unsigned char status;		/* [o] scsi status */
> -	unsigned char masked_status;	/* [o] shifted, masked scsi status */
> -	unsigned char msg_status;	/* [o] messaging level data (optional) */
> -	unsigned char sb_len_wr;	/* [o] byte count actually written to sbp */
> -	unsigned short host_status;	/* [o] errors from host adapter */
> -	unsigned short driver_status;	/* [o] errors from software driver */
> -	compat_int_t resid;		/* [o] dxfer_len - actual_transferred */
> -	compat_uint_t duration;		/* [o] time taken by cmd (unit: millisec) */
> -	compat_uint_t info;		/* [o] auxiliary information */
> -};
> -#endif
> -
>  int put_sg_io_hdr(const struct sg_io_hdr *hdr, void __user *argp)
>  {
>  #ifdef CONFIG_COMPAT
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 160748ad9c0f..985546aac236 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -198,6 +198,7 @@ static void sg_device_destroy(struct kref *kref);
>  
>  #define SZ_SG_HEADER sizeof(struct sg_header)
>  #define SZ_SG_IO_HDR sizeof(sg_io_hdr_t)
> +#define SZ_COMPAT_SG_IO_HDR sizeof(struct compat_sg_io_hdr)

I'd rather not add more defines like this.  The raw sizeof is
much more readable and obvious.

>  
> +	if (count < SZ_SG_HEADER)
> +		goto unknown_id;
> +
> +	/* negative reply_len means v3 format, otherwise v1/v2 */
> +	if (get_user(reply_len, &old_hdr->reply_len))
> +		return -EFAULT;
> +	if (reply_len >= 0)
> +		return get_user(*pack_id, &old_hdr->pack_id);
> +
> +	if (in_compat_syscall() && count >= SZ_COMPAT_SG_IO_HDR) {
> +		struct compat_sg_io_hdr __user *hp = buf;
> +		return get_user(*pack_id, &hp->pack_id);
> +	}
> +
> +	if (count >= SZ_SG_IO_HDR) {
> +		struct sg_io_hdr __user *hp = buf;
> +		return get_user(*pack_id, &hp->pack_id);
> +	}
> +
> +unknown_id:
> +	/* no valid header was passed, so ignore the pack_id */
> +	*pack_id = -1;
> +	return 0;
> +}

I find the structure here a little confusing, as it doesn't follow
the normal flow.  What do you think of:

	if (count >= SZ_SG_HEADER) {
		if (get_user(reply_len, &old_hdr->reply_len))
			return -EFAULT;

		/* negative reply_len means v3 format, otherwise v1/v2 */
		if (reply_len >= 0)
			return get_user(*pack_id, &old_hdr->pack_id);

		if (in_compat_syscall() {
			if (count >= SZ_COMPAT_SG_IO_HDR) {
				struct compat_sg_io_hdr __user *hp = buf;

				return get_user(*pack_id, &hp->pack_id);
			}
		} else {
			if (count >= SZ_SG_IO_HDR) {
				struct sg_io_hdr __user *hp = buf;

				return get_user(*pack_id, &hp->pack_id);
			}
		}
	}

	/* no valid header was passed, so ignore the pack_id */
	*pack_id = -1;
	return 0;
}

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-11 20:42 [PATCH 00/24] block, scsi: final compat_ioctl cleanup Arnd Bergmann
2019-12-11 20:42 ` [PATCH 02/24] compat: scsi: sg: fix v3 compat read/write interface Arnd Bergmann
2019-12-12 16:25   ` Christoph Hellwig [this message]
2019-12-12 17:34     ` Arnd Bergmann
2019-12-11 20:42 ` [PATCH 03/24] compat_ioctl: block: handle BLKREPORTZONE/BLKRESETZONE Arnd Bergmann
2019-12-12  1:20   ` Damien Le Moal
2019-12-11 20:42 ` [PATCH 04/24] compat_ioctl: block: handle BLKGETZONESZ/BLKGETNRZONES Arnd Bergmann
2019-12-12  1:20   ` Damien Le Moal
2019-12-11 20:42 ` [PATCH 05/24] compat_ioctl: block: handle add zone open, close and finish ioctl Arnd Bergmann
2019-12-12  1:20   ` Damien Le Moal
2019-12-11 20:42 ` [PATCH 06/24] compat_ioctl: block: handle Persistent Reservations Arnd Bergmann
2019-12-11 20:42 ` [PATCH 07/24] compaT_ioctl: ubd, aoe: use blkdev_compat_ptr_ioctl Arnd Bergmann
2019-12-11 20:42 ` [PATCH 08/24] compat_ioctl: move CDROM_SEND_PACKET handling into scsi Arnd Bergmann
2019-12-11 20:42 ` [PATCH 09/24] compat_ioctl: move CDROMREADADIO to cdrom.c Arnd Bergmann
2019-12-11 20:42 ` [PATCH 10/24] compat_ioctl: cdrom: handle CDROM_LAST_WRITTEN Arnd Bergmann
2019-12-17 15:20   ` [Y2038] " Ben Hutchings
2019-12-17 21:38     ` Arnd Bergmann
2019-12-11 20:42 ` [PATCH 11/24] compat_ioctl: block: handle cdrom compat ioctl in non-cdrom drivers Arnd Bergmann
2019-12-11 20:42 ` [PATCH 13/24] compat_ioctl: bsg: add handler Arnd Bergmann
2019-12-11 20:42 ` [PATCH 15/24] compat_ioctl: scsi: move ioctl handling into drivers Arnd Bergmann
2019-12-11 23:05   ` Michael S. Tsirkin
2019-12-12  0:28     ` Paolo Bonzini
2019-12-12  9:17       ` Arnd Bergmann
2019-12-12 10:27       ` Michael S. Tsirkin
2019-12-12 16:27       ` Christoph Hellwig
2019-12-12 16:35         ` Jens Axboe
2019-12-12 10:25   ` Michael S. Tsirkin
2019-12-11 20:42 ` [PATCH 18/24] compat_ioctl: move cdrom commands into cdrom.c Arnd Bergmann
2019-12-11 20:42 ` [PATCH 20/24] compat_ioctl: move HDIO ioctl handling into drivers/ide Arnd Bergmann
2019-12-12 16:29   ` Christoph Hellwig
2019-12-12 17:21     ` Arnd Bergmann
2019-12-11 20:42 ` [PATCH 21/24] compat_ioctl: block: move blkdev_compat_ioctl() into ioctl.c Arnd Bergmann
2019-12-12 16:30   ` Christoph Hellwig
2019-12-12 17:24     ` Arnd Bergmann
2019-12-11 20:42 ` [PATCH 22/24] compat_ioctl: block: simplify compat_blkpg_ioctl() Arnd Bergmann
2019-12-11 20:42 ` [PATCH 23/24] compat_ioctl: simplify up block/ioctl.c Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191212162506.GA27991@infradead.org \
    --to=hch@infradead.org \
    --cc=arnd@arndb.de \
    --cc=axboe@kernel.dk \
    --cc=chaitanya.kulkarni@wdc.com \
    --cc=dgilbert@interlog.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jejb@linux.ibm.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=maier@linux.ibm.com \
    --cc=martin.petersen@oracle.com \
    --cc=tglx@linutronix.de \
    --cc=viro@zeniv.linux.org.uk \
    --cc=y2038@lists.linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git