Hello Emil,
thanks for your review.
I have understand most of your remarks and I'm fixing them
but some points aren't obvious for me...
2016-05-17 0:58 GMT+02:00 Emil Velikov <emil.l.velikov@xxxxxxxxx>:
> Hi Benjamin,
>
> I'd suspect you're interested in some feedback on these, so here is a few :-)
> Sadly (ideally?) nothing serious, but a bunch minor suggestions, plus
> the odd bug.
>
> On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx> wrote:
>
>> --- /dev/null
>> +++ b/drivers/smaf/smaf-core.c
>> @@ -0,0 +1,794 @@
>> +/*
>> + * smaf.c
> The comment does not match the actual file name.
>
> You could give a brief summary of the file(s), if you're feeling gracious ;-)
>
>
>> +
>> +/**
>> + * smaf_grant_access - return true if the specified device can get access
>> + * to the memory area
>> + *
> Reading this makes me wonder if {request,allow}_access won't be better name ?
>
grant and revoke sound more secure oriented for me but that could change
>
>> +static int smaf_secure_handle(struct smaf_handle *handle)
>> +{
>> + if (atomic_read(&handle->is_secure))
>> + return 0;
>> +
>> + if (!have_secure_module())
>> + return -EINVAL;
>> +
>> + handle->secure_ctx = smaf_dev.secure->create_ctx();
>> +
> Should one use a temporary variable so that the caller provided
> storage is unchanged in case of an error ?
>
>> + if (!handle->secure_ctx)
>> + return -EINVAL;
>> +
>> + atomic_set(&handle->is_secure, 1);
>> + return 0;
>> +}
>> +
>
>
>> +int smaf_register_secure(struct smaf_secure *s)
>> +{
>> + /* make sure that secure module have all required functions
>> + * to avoid test them each time later
>> + */
>> + WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
>> + !s->grant_access || !s->revoke_access);
>> +
> Is something like below reasonable thing to do in the kernel ?
> Same question goes for smaf_register_allocator() further down.
>
> if (!s || ....)
> return -ESHOULDNEVERHAPPEN;
>
>
>
>> +static struct vm_operations_struct smaf_vma_ops = {
> Ops/vfucs normally are const data. Is there something preventing us here ?
>
>> + .close = smaf_vm_close,
>> +};
>> +
>> +static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>> +{
>> + struct smaf_handle *handle = dmabuf->priv;
>> + bool ret;
>> + enum dma_data_direction dir;
>> +
>> + /* if no allocator attached, get the first allocator */
>> + if (!handle->allocator) {
>> + struct smaf_allocator *alloc;
>> +
>> + mutex_lock(&smaf_dev.lock);
>> + alloc = smaf_get_first_allocator(dmabuf);
>> + mutex_unlock(&smaf_dev.lock);
>> +
>> + /* still no allocator ? */
>> + if (!alloc)
>> + return -EINVAL;
>> +
>> + handle->allocator = alloc;
>> + }
>> +
>> + if (!handle->db_alloc) {
>> + struct dma_buf *db_alloc;
>> +
>> + db_alloc = handle->allocator->allocate(dmabuf,
>> + handle->length,
>> + handle->flags);
>> + if (!db_alloc)
>> + return -EINVAL;
>> +
>> + handle->db_alloc = db_alloc;
>> + }
>> +
> The above half of the function looks identical to smaf_map_dma_buf().
> Worth factoring it out to a helper function ?
>
>
>> +static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
>> + struct dma_buf_attachment *attach)
>> +{
>> + struct smaf_handle *handle = dmabuf->priv;
>> + struct dma_buf_attachment *db_attach;
>> +
>> + if (!handle->db_alloc)
>> + return 0;
>> +
> Shouldn't one return an error (-EINVAL or similar) here ?
No because a device could attach itself on the buffer and the
allocator will only
be selected at the first map_attach call.
The goal is to delay the allocation until all devices are attached to
select the best allocator.
>
>
>> +static struct dma_buf_ops smaf_dma_buf_ops = {
> const ? From a very quick look the compiler should warn us about it -
> "smaf_dma_buf_ops discards const qualifier" or similar.
>
>
>> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
>> +{
>> + struct smaf_handle *handle;
>> +
>> + DEFINE_DMA_BUF_EXPORT_INFO(info);
>> +
>> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>> + if (!handle)
>> + return ERR_PTR(-ENOMEM);
>> +
> Err this should be return NULL; correct ?
>
>> + info.ops = &smaf_dma_buf_ops;
>> + info.size = round_up(length, PAGE_SIZE);
>> + info.flags = flags;
>> + info.priv = handle;
>> +
>> + handle->dmabuf = dma_buf_export(&info);
>> + if (IS_ERR(handle->dmabuf)) {
>> + kfree(handle);
>> + return NULL;
>> + }
>> +
>> + handle->length = info.size;
>> + handle->flags = flags;
>> +
>> + return handle;
>> +}
>> +EXPORT_SYMBOL(smaf_create_handle);
>> +
>> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> +{
>> + switch (cmd) {
>> + case SMAF_IOC_CREATE:
>> + {
>> + struct smaf_create_data data;
>> + struct smaf_handle *handle;
>> +
>> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>> + return -EFAULT;
>> +
>> + handle = smaf_create_handle(data.length, data.flags);
> We want to sanitise the input data.{length,flags} before sending it
> deeper in the kernel.
Sorry but can you elaborate little more here ?
I don't understand your expectations.
>
>> + if (!handle)
>> + return -EINVAL;
>> +
>> + if (data.name[0]) {
>> + /* user force allocator selection */
>> + if (smaf_select_allocator_by_name(handle->dmabuf,
>> + data.name)) {
>> + dma_buf_put(handle->dmabuf);
> Missing free(handle), here and through the rest of the case statement ?
>
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + handle->fd = dma_buf_fd(handle->dmabuf, data.flags);
>> + if (handle->fd < 0) {
> Worth adding smaf_DEselect_allocator_by_name() and using it here + below ?
>
>> + dma_buf_put(handle->dmabuf);
>> + return -EINVAL;
>> + }
>> +
>> + data.fd = handle->fd;
>> + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
>> + dma_buf_put(handle->dmabuf);
>> + return -EFAULT;
>> + }
>> + break;
>> + }
>> + case SMAF_IOC_GET_SECURE_FLAG:
>> + {
>> + struct smaf_secure_flag data;
>> + struct dma_buf *dmabuf;
>> +
>> + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>> + return -EFAULT;
>> +
>> + dmabuf = dma_buf_get(data.fd);
> Worth adding if (data.fd == -1) return -EINVAL; ?
>
>
>
>> +
>> +static const struct file_operations smaf_fops = {
>> + .owner = THIS_MODULE,
> There was a recent 'crusade' to get rid of these. Are you sure we
> want/need this ?
>
>> + .unlocked_ioctl = smaf_ioctl,
>> +};
>> +
>> +static int __init smaf_init(void)
>> +{
>> + int ret = 0;
>> +
> Please drop the default initialization.
>
>> + smaf_dev.misc_dev.minor = MISC_DYNAMIC_MINOR;
>> + smaf_dev.misc_dev.name = "smaf";
>> + smaf_dev.misc_dev.fops = &smaf_fops;
>> +
> Initialize the global static variable (smaf_dev) upon declaration ?
>
>
>> --- /dev/null
>> +++ b/include/linux/smaf-secure.h
>
>> +/**
>> + * smaf_create_handle - create a smaf_handle with the give length and flags
>> + * do not allocate memory but provide smaf_handle->dmabuf that can be
>> + * shared between devices.
>> + *
>> + * @length: buffer size
>> + * @flags: handle flags
>> + */
>> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags);
>> +
> Inspired by the bug (?) in this function I think you want to document
> the return value throughout the header.
It is useless the add this function in this .h file I will remove it
and fix the comment in structure defintion
>
>> +#endif
>> diff --git a/include/uapi/linux/smaf.h b/include/uapi/linux/smaf.h
>> new file mode 100644
>> index 0000000..5a9201b
>> --- /dev/null
>> +++ b/include/uapi/linux/smaf.h
>> @@ -0,0 +1,66 @@
>> +/*
>> + * smaf.h
>> + *
> Would be nice if we had more elaborate comment in an UAPI header.
>
>
>> +/**
>> + * struct smaf_create_data - allocation parameters
>> + * @length: size of the allocation
>> + * @flags: flags passed to allocator
>> + * @name: name of the allocator to be selected, could be NULL
> Is it guaranteed to be null terminated ? If so one should mentioned it
> otherwise your userspace should be fixed.
> Same comments apply for smaf_info::name.
I have used strncpy everywhere to avoid this problem but maybe it is not enough
>
>
>> + * @fd: returned file descriptor
>> + */
>> +struct smaf_create_data {
>> + size_t length;
>> + unsigned int flags;
>> + char name[ALLOCATOR_NAME_LENGTH];
>> + int fd;
> The structs here feels quite fragile. Please read up on Daniel
> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
> useful is such process.
>
if I describe the structures like this:
/**
* struct smaf_create_data - allocation parameters
* @length: size of the allocation
* @flags: flags passed to allocator
* @name_len: length of name
* @name: name of the allocator to be selected, could be NULL
* @fd: returned file descriptor
*/
struct smaf_create_data {
size_t length;
unsigned int flags;
size_t name_len;
char __user *name;
int fd;
char padding[44];
};
does it sound more robust for you ?
> Hopefully I haven't lost the plot with the above, if I had don't be
> shy to point out.
>
> Thanks,
> Emil
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
--
Benjamin Gaignard
Graphic Working Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
