2017-09-19 12:15 GMT+02:00 Tomas Winkler <tom...@gmail.com>:
> On Tue, Sep 19, 2017 at 1:07 PM, Benjamin Gaignard
> <benjamin.gaign...@linaro.org> wrote:
>> 2017-09-19 11:40 GMT+02:00 Dan Carpenter <dan.carpen...@oracle.com>:
>>> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
>>>> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>>> +static int validate_ioctl_arg(struct file *filp,
>>>> + unsigned int cmd, union ion_ioctl_arg *arg)
>>>> {
>>>> int ret = 0;
>>>> + int mask = 1 << iminor(filp->f_inode);
>>>>
>>>> switch (cmd) {
>>>> case ION_IOC_HEAP_QUERY:
>>>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union
>>>> ion_ioctl_arg *arg)
>>>> ret |= arg->query.reserved1 != 0;
>>>> ret |= arg->query.reserved2 != 0;
>>>> break;
>>>> + case ION_IOC_ALLOC:
>>>> + ret = !(arg->allocation.heap_id_mask & mask);
>>>
>>>
>>> validate_ioctl_arg() is really convoluted. From reading just the patch
>>> I at first thought we were returning 1 on failure. Just say:
>>>
>>> if (!(arg->allocation.heap_id_mask & mask))
>>> return -EINVAL;
>>> return 0;
>>>
>>> If you want to fix the surrounding code in a separate patch that would
>>> be good. It would be more clear to say:
>>>
>>> if (arg->query.reserved0 != 0 ||
>>> arg->query.reserved1 != 0 ||
>>> arg->query.reserved2 != 0)
>>> return -EINVAL;
>>
>> I agree I will add a fix for that in next version
>>
>>>
>>>> + break;
>>>> default:
>>>> break;
>>>> }
>>>> @@ -70,7 +75,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>>>> unsigned long arg)
>>>> if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>>>> return -EFAULT;
>>>>
>>>> - ret = validate_ioctl_arg(cmd, &data);
>>>> + ret = validate_ioctl_arg(filp, cmd, &data);
>>>> if (WARN_ON_ONCE(ret))
>>>> return ret;
>>>>
>>>> diff --git a/drivers/staging/android/ion/ion.c
>>>> b/drivers/staging/android/ion/ion.c
>>>> index 93e2c90..5144f1a 100644
>>>> --- a/drivers/staging/android/ion/ion.c
>>>> +++ b/drivers/staging/android/ion/ion.c
>>>> @@ -40,6 +40,8 @@
>>>>
>>>> #include "ion.h"
>>>>
>>>> +#define ION_DEV_MAX 32
>>>> +
>>>> static struct ion_device *internal_dev;
>>>> static int heap_id;
>>>>
>>>> @@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
>>>> {
>>>> struct dentry *debug_file;
>>>> struct ion_device *dev = internal_dev;
>>>> + int ret;
>>>>
>>>> if (!heap->ops->allocate || !heap->ops->free)
>>>> pr_err("%s: can not add heap with invalid ops struct.\n",
>>>> __func__);
>>>>
>>>
>>> I don't think it can happen in current code but we should proabably have
>>> a check here for:
>>>
>>> if (heap_id >= ION_DEV_MAX)
>>> return -EBUSY;
>>>
>>> (It's possible I have missed something).
>>>
>>
>> You are right I will add that
>>
>> Thanks
>>>
>>>> + heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
>>>> + dev_set_name(&heap->ddev, "ion%d", heap_id);
>>>> + device_initialize(&heap->ddev);
>>>> + cdev_init(&heap->chrdev, &ion_fops);
>>>> + heap->chrdev.owner = THIS_MODULE;
>>>> + ret = cdev_device_add(&heap->chrdev, &heap->ddev);
>>>> + if (ret < 0)
>>>> + return;
>>>> +
>>>> spin_lock_init(&heap->free_lock);
>>>> heap->free_list_size = 0;
>
> What will happen to an application which looks for /dev/ion?
/dev/ion will no more exist with this patch.
Since ion ABI have already change a lot I don't think that could
be a problem to change also ion device.
>
> Thanks
> Tomas
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
