Re: [PATCH v2] staging: comedi: change do_insn*_ioctl to allow more samples

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 4, 2018 at 12:08 PM Spencer E. Olson <olsonse@xxxxxxxxx> wrote:
>
> Changes do_insn*_ioctl functions to allow for data lengths for each
> comedi_insn of up to 2^16.  This patch also changes these functions to only
> allocate as much memory as is necessary for each comedi_insn, rather than
> allocating a fixed-sized scratch space.
>
> In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
> facility with some newer hardware, I discovered that do_insn_ioctl and
> do_insnlist_ioctl limited the amount of data that can be passed into the
> kernel for insn's to a length of 256.  For some newer hardware, the number
> of routes can be greater than 1000.  Working around the old limits (256)
> would complicate the user-space/kernel interaction.
>
> The new upper limit is reasonable with current memory available and does
> not otherwise impact the memory footprint for any current or otherwise
> typical configuration.
>
> Signed-off-by: Spencer E. Olson <olsonse@xxxxxxxxx>
> ---
> Implements Ian's suggestions to:
> 1) Provide a minimum amount of space to allocate (16*sizeof(uint)) to protect
>    drivers that do not (yet) check the size of the instruction data (Ian has
>    submitted several patches fixing this for other drivers already).  In case
>    insn.n does not get set, this minimum amount also avoids trying to allocate
>    zero-length data.
> 2) Allocate the maximum required space needed for any of the instructions in an
>    instruction list before executing the list of instructions (this, rather than
>    allocating and freeing memory separately while iterating through the
>    instruction list and executing each instruction).
>
>  drivers/staging/comedi/comedi_fops.c | 48 ++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index ceb6ba5dd57c..5d2fcbfe02af 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1501,25 +1501,21 @@ static int parse_insn(struct comedi_device *dev, struct comedi_insn *insn,
>   *     data (for reads) to insns[].data pointers
>   */
>  /* arbitrary limits */
> -#define MAX_SAMPLES 256
> +#define MIN_SAMPLES 16
> +#define MAX_SAMPLES 65536
>  static int do_insnlist_ioctl(struct comedi_device *dev,
>                              struct comedi_insnlist __user *arg, void *file)
>  {
>         struct comedi_insnlist insnlist;
>         struct comedi_insn *insns = NULL;
>         unsigned int *data = NULL;
> +       unsigned int max_n_data_required = MIN_SAMPLES;
>         int i = 0;
>         int ret = 0;
>
>         if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
>                 return -EFAULT;
>
> -       data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> -       if (!data) {
> -               ret = -ENOMEM;
> -               goto error;
> -       }
> -
>         insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
>         if (!insns) {
>                 ret = -ENOMEM;
> @@ -1533,13 +1529,26 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
>                 goto error;
>         }
>
> -       for (i = 0; i < insnlist.n_insns; i++) {
> +       /* Determine maximum memory needed for all instructions. */
> +       for (i = 0; i < insnlist.n_insns; ++i) {
>                 if (insns[i].n > MAX_SAMPLES) {
>                         dev_dbg(dev->class_dev,
>                                 "number of samples too large\n");
>                         ret = -EINVAL;
>                         goto error;
>                 }
> +               max_n_data_required = max(max_n_data_required, insns[i].n);
> +       }

I realized just now that this patch does change behavior just bit:
the complaint, error, and exit are done _before_ any instruction is
executed, rather than after the prior instructions in the list are
executed.  I argue this is actually a better behavior than partially
executing an instruction list, especially since this pre-inspection
could have already easily been done before.

> +
> +       /* Allocate scratch space for all instruction data. */
> +       data = kmalloc_array(max_n_data_required, sizeof(unsigned int),
> +                            GFP_KERNEL);
> +       if (!data) {
> +               ret = -ENOMEM;
> +               goto error;
> +       }
> +
> +       for (i = 0; i < insnlist.n_insns; ++i) {
>                 if (insns[i].insn & INSN_MASK_WRITE) {
>                         if (copy_from_user(data, insns[i].data,
>                                            insns[i].n * sizeof(unsigned int))) {
> @@ -1593,22 +1602,27 @@ static int do_insn_ioctl(struct comedi_device *dev,
>  {
>         struct comedi_insn insn;
>         unsigned int *data = NULL;
> +       unsigned int n_data = MIN_SAMPLES;
>         int ret = 0;
>
> -       data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> -       if (!data) {
> -               ret = -ENOMEM;
> -               goto error;
> -       }
> -
>         if (copy_from_user(&insn, arg, sizeof(insn))) {
> -               ret = -EFAULT;
> -               goto error;
> +               return -EFAULT;
>         }
>
> +       n_data = max(n_data, insn.n);
> +
>         /* This is where the behavior of insn and insnlist deviate. */
> -       if (insn.n > MAX_SAMPLES)
> +       if (insn.n > MAX_SAMPLES) {
>                 insn.n = MAX_SAMPLES;
> +               n_data = MAX_SAMPLES;
> +       }
> +
> +       data = kmalloc_array(n_data, sizeof(unsigned int), GFP_KERNEL);
> +       if (!data) {
> +               ret = -ENOMEM;
> +               goto error;
> +       }
> +
>         if (insn.insn & INSN_MASK_WRITE) {
>                 if (copy_from_user(data,
>                                    insn.data,
> --
> 2.17.1
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux