Re: [PATCH 1/4] scsi: sg: fix BLKSECTGET ioctl

From: Douglas Gilbert <dgilbert@interlog.com>
To: Bart Van Assche <bvanassche@acm.org>,
	Tom Yan <tom.ty89@gmail.com>,
	linux-scsi@vger.kernel.org
Cc: stern@rowland.harvard.edu, James.Bottomley@SteelEye.com,
	akinobu.mita@gmail.com, hch@lst.de, jens.axboe@oracle.com
Subject: Re: [PATCH 1/4] scsi: sg: fix BLKSECTGET ioctl
Date: Sat, 5 Sep 2020 16:36:57 -0400	[thread overview]
Message-ID: <868195a0-94f2-f009-bfd4-f206f0da7db8@interlog.com> (raw)
In-Reply-To: <caf0aa91-3e54-f8bd-d818-587f4318716c@acm.org>

On 2020-09-05 3:32 p.m., Bart Van Assche wrote:
> On 2020-09-04 13:05, Tom Yan wrote:
>> It should give out the maximum number of sectors per request
>> instead of maximum number of bytes.
>>
>> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>> ---
>>   drivers/scsi/sg.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 20472aaaf630..e57831910228 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -922,6 +922,7 @@ sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,
>>   	int result, val, read_only;
>>   	Sg_request *srp;
>>   	unsigned long iflags;
>> +	unsigned int max_sectors;
>>   
>>   	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
>>   				   "sg_ioctl: cmd=0x%x\n", (int) cmd_in));
>> @@ -1114,8 +1115,9 @@ sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,
>>   		sdp->sgdebug = (char) val;
>>   		return 0;
>>   	case BLKSECTGET:
>> -		return put_user(max_sectors_bytes(sdp->device->request_queue),
>> -				ip);
>> +		max_sectors = min_t(unsigned int, USHRT_MAX,
>> +				    queue_max_sectors(sdp->device->request_queue));
>> +		return put_user(max_sectors, ip);
>>   	case BLKTRACESETUP:
>>   		return blk_trace_setup(sdp->device->request_queue,
>>   				       sdp->disk->disk_name,
> 
> Is this perhaps a backwards-incompatible change to the kernel ABI, the
> kind of change Linus totally disagrees with?
> 
> Additionally, please Cc linux-api for patches that modify the kernel ABI.
>>From https://www.kernel.org/doc/man-pages/linux-api-ml.html: "The kernel
> source file Documentation/SubmitChecklist notes that all Linux kernel
> patches that change userspace interfaces should be CCed to
> linux-api@vger.kernel.org, so that the various parties who are interested
> in API changes are informed. For further information, see
> https://www.kernel.org/doc/man-pages/linux-api-ml.html"

Hmm,
The BLK* ioctl()s in the sg driver were an undocumented addition by others.
Plus it is not clear to me why a char device such as the sg driver should
be supporting BLK* ioctl(2)s. For example, how should an enclosure react to 
those ioctls or a WLUN?

If they are going to be supported for storage devices and /dev/sdb and
/dev/sg2 are the same device then if:
    blockdev --getmaxsect /dev/sg1

gives a different result to:
    blockdev --getmaxsect /dev/sdb

then I would consider that a bug. So fixing it is making the kernel ABI
more consistent :-)

Doug Gilbert