Re: [PATCH v2] staging: comedi: change do_insn*_ioctl to allow more samples
On 04/12/2018 19:07, Spencer E. Olson 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 --- 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(-) Looks good, thanks! (I still need to crack on and fix up the remaining drivers that ignore insn->n.) Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: comedi: change do_insn*_ioctl to allow more samples
On Tue, Dec 4, 2018 at 12:08 PM Spencer E. Olson 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
> ---
> 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) {
> -
[PATCH v2] staging: comedi: change do_insn*_ioctl to allow more samples
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
---
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);
+ }
+
+ /* 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;
+ }
+
