On Wednesday 07 October 2015 10:54:03 Yaniv Gardi wrote:
>
> +/* IOCTL opcode for command - ufs set device read only */
> +#define UFS_IOCTL_BLKROSET BLKROSET
> +
What is this for? Can't you just use the normal BLKROSET definition in user space?
> +
> + ioctl_data = kzalloc(sizeof(struct ufs_ioctl_query_data), GFP_KERNEL);
> + if (!ioctl_data) {
> + err = -ENOMEM;
> + goto out;
> + }
ufs_ioctl_query_data is short enough to just be on the stack.
> + }
> + length = min_t(int, QUERY_DESC_MAX_SIZE,
> + ioctl_data->buf_size);
> + desc = kzalloc(length, GFP_KERNEL);
> + if (!desc) {
> + dev_err(hba->dev, "%s: Failed allocating %d bytes\n",
> + __func__, length);
> + err = -ENOMEM;
> + goto out_release_mem;
> + }
Better check for a maximum length as well, not just a minimum length.
For overly long requests, just return an error without trying the kzalloc.
You can also use memdup_user to avoid the extra zeroing and and simplify
the calling code.
> +/**
> + * ufshcd_ioctl - ufs ioctl callback registered in scsi_host
> + * @dev: scsi device required for per LUN queries
> + * @cmd: command opcode
> + * @buffer: user space buffer for transferring data
> + *
> + * Supported commands:
> + * UFS_IOCTL_QUERY
> + */
> +static int ufshcd_ioctl(struct scsi_device *dev, int cmd, void __user *buffer)
> +{
> + struct ufs_hba *hba = shost_priv(dev->host);
> + int err = 0;
> +
> + BUG_ON(!hba);
> + if (!buffer) {
> + dev_err(hba->dev, "%s: User buffer is NULL!\n", __func__);
> + return -EINVAL;
> + }
Don't print an error here, you don't want users to be able to flood
syslog that easily, and the program that caused the error does not
see the log anyway.
> +
> + switch (cmd) {
> + case UFS_IOCTL_QUERY:
> + pm_runtime_get_sync(hba->dev);
> + err = ufshcd_query_ioctl(hba, ufshcd_scsi_to_upiu_lun(dev->lun),
> + buffer);
> + pm_runtime_put_sync(hba->dev);
> + break;
> + case UFS_IOCTL_BLKROSET:
> + err = -ENOIOCTLCMD;
> + break;
> + default:
> + err = -EINVAL;
> + dev_err(hba->dev, "%s: Illegal ufs-IOCTL cmd %d\n", __func__,
> + cmd);
> + break;
same here.
> + }
> +
> + return err;
> +}
> +
> +/**
> * ufshcd_async_scan - asynchronous execution for probing hba
> * @data: data pointer to pass to this function
> * @cookie: cookie data
> @@ -5106,6 +5322,7 @@ static struct scsi_host_template ufshcd_driver_template = {
> .eh_device_reset_handler = ufshcd_eh_device_reset_handler,
> .eh_host_reset_handler = ufshcd_eh_host_reset_handler,
> .eh_timed_out = ufshcd_eh_timed_out,
> + .ioctl = ufshcd_ioctl,
> .this_id = -1,
> .sg_tablesize = SG_ALL,
> .cmd_per_lun = UFSHCD_CMD_PER_LUN,
The ioctl data is compatible between 32-bit and 64-bit user space, so
better add a .compat_ioctl line right away to make this work on 64-bit
architectures with 32-bit user space.
> +
> +/*
> + * IOCTL opcode for ufs queries has the following opcode after
> + * SCSI_IOCTL_GET_PCI
> + */
> +#define UFS_IOCTL_QUERY 0x5388
Use _IOWR() to define that number with the correct argument length
> +/**
> + * struct ufs_ioctl_query_data - used to transfer data to and from
> + * user via ioctl
> + * @opcode: type of data to query (descriptor/attribute/flag)
> + * @idn: id of the data structure
> + * @buf_size: number of allocated bytes/data size on return
> + * @buffer: data location
> + *
> + * Received: buffer and buf_size (available space for transferred data)
> + * Submitted: opcode, idn, length, buf_size
> + */
> +struct ufs_ioctl_query_data {
> + /*
> + * User should select one of the opcode defined in "enum query_opcode".
> + * Please check include/uapi/scsi/ufs/ufs.h for the definition of it.
> + * Note that only UPIU_QUERY_OPCODE_READ_DESC,
> + * UPIU_QUERY_OPCODE_READ_ATTR & UPIU_QUERY_OPCODE_READ_FLAG are
> + * supported as of now. All other query_opcode would be considered
> + * invalid.
> + * As of now only read query operations are supported.
> + */
> + __u32 opcode;
> + /*
> + * User should select one of the idn from "enum flag_idn" or "enum
> + * attr_idn" or "enum desc_idn" based on whether opcode above is
> + * attribute, flag or descriptor.
> + * Please check include/uapi/scsi/ufs/ufs.h for the definition of it.
> + */
> + __u8 idn;
> + /*
> + * User should specify the size of the buffer (buffer[0] below) where
> + * it wants to read the query data (attribute/flag/descriptor).
> + * As we might end up reading less data then what is specified in
> + * buf_size. So we are updating buf_size to what exactly we have read.
> + */
> + __u16 buf_size;
> + /*
> + * placeholder for the start of the data buffer where kernel will copy
> + * the query data (attribute/flag/descriptor) read from the UFS device
> + * Note:
> + * For Read Attribute you will have to allocate 4 bytes
> + * For Read Flag you will have to allocate 1 byte
> + */
> + __u8 buffer[0];
> +};
Better rearrange the structure to avoid the implied padding, either by
making idn a __u16, or by adding an explicit __u8 member behind it.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
