Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space

[Sakari Ailus]

On Sun, Jul 10, 2016 at 02:12:24AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sunday 10 Jul 2016 01:03:09 Sakari Ailus wrote:
> > On Sat, Jul 09, 2016 at 10:29:03PM +0300, Laurent Pinchart wrote:
> > > On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> > >> Laurent Pinchart wrote:
> > >>> On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
> >  Refactor copying the IOCTL argument structs from the user space and
> >  back, in order to reduce code copied around and make the
> >  implementation more robust.
> >  
> >  As a result, the copying is done while not holding the graph mutex.
> >  
> >  Signed-off-by: Sakari Ailus 
> >  ---
> >  since v2:
> >  
> >  - Remove function to calculate maximum argument size, replace by a
> >    char array of 256 or kmalloc() if that's too small.
> >   
> >   drivers/media/media-device.c | 194 ++---
> >   1 file changed, 94 insertions(+), 100 deletions(-)
> >  
> >  diff --git a/drivers/media/media-device.c
> >  b/drivers/media/media-device.c
> >  index 9b5a88d..0797e4b 100644
> >  --- a/drivers/media/media-device.c
> >  +++ b/drivers/media/media-device.c
> > > 
> > > [snip]
> > > 
> >  @@ -453,10 +432,24 @@ static long __media_device_ioctl(
> >  
> > info = _array[_IOC_NR(cmd)];
> >  
> >  +  if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
> >  +  karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
> >  +  if (!karg)
> >  +  return -ENOMEM;
> >  +  }
> >  +
> >  +  info->arg_from_user(karg, arg, cmd);
> >  +
> > mutex_lock(>graph_mutex);
> >  -  ret = info->fn(dev, arg);
> >  +  ret = info->fn(dev, karg);
> > mutex_unlock(>graph_mutex);
> >  
> >  +  if (!ret)
> > >>> 
> > >>> How about if (!ret && info->arg_to_user) instead, and getting rid of
> > >>> copy_arg_to_user_nop() ?
> > >> 
> > >> I thought of that, but I decided to optimise the common case ---  which
> > >> is that the argument is copied back and forth. Not copying the argument
> > >> back is a very special case, we use it for a single compat IOCTL.
> > >> 
> > >> That said, we could use it for the proper ENUM_LINKS as well. Still that
> > >> does not change what's normal.
> > > 
> > > We're talking about one comparison and one branching instruction (that
> > > will not be taken in the common case). Is that micro-optimization really
> > > worth it in an ioctl path that is not that performance-critical ? If you
> > > think it is, could you analyse what the impact of the
> > > copy_arg_to_user_nop() function on cache locality is for the common case ?
> > > ;-)
> > 
> > I sense a certain amount of insistence in your arguments. Fine, I'll change
> > it.
> 
> Thanks. I'll change that in the next version of the request API patches I 
> will 
> send out.

I think we rather should try to decrease the size of the set and get the
preparation patches in first.

I'm ready to send a pull request on these (after testing the rebased
patches), but it's been pending on the minimum arg size vs. list of
supported sizes discussion.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space

[Laurent Pinchart]

Hi Sakari,

On Sunday 10 Jul 2016 01:03:09 Sakari Ailus wrote:
> On Sat, Jul 09, 2016 at 10:29:03PM +0300, Laurent Pinchart wrote:
> > On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> >> Laurent Pinchart wrote:
> >>> On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
>  Refactor copying the IOCTL argument structs from the user space and
>  back, in order to reduce code copied around and make the
>  implementation more robust.
>  
>  As a result, the copying is done while not holding the graph mutex.
>  
>  Signed-off-by: Sakari Ailus 
>  ---
>  since v2:
>  
>  - Remove function to calculate maximum argument size, replace by a
>    char array of 256 or kmalloc() if that's too small.
>   
>   drivers/media/media-device.c | 194 ++---
>   1 file changed, 94 insertions(+), 100 deletions(-)
>  
>  diff --git a/drivers/media/media-device.c
>  b/drivers/media/media-device.c
>  index 9b5a88d..0797e4b 100644
>  --- a/drivers/media/media-device.c
>  +++ b/drivers/media/media-device.c
> > 
> > [snip]
> > 
>  @@ -453,10 +432,24 @@ static long __media_device_ioctl(
>  
>   info = _array[_IOC_NR(cmd)];
>  
>  +if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
>  +karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
>  +if (!karg)
>  +return -ENOMEM;
>  +}
>  +
>  +info->arg_from_user(karg, arg, cmd);
>  +
>   mutex_lock(>graph_mutex);
>  -ret = info->fn(dev, arg);
>  +ret = info->fn(dev, karg);
>   mutex_unlock(>graph_mutex);
>  
>  +if (!ret)
> >>> 
> >>> How about if (!ret && info->arg_to_user) instead, and getting rid of
> >>> copy_arg_to_user_nop() ?
> >> 
> >> I thought of that, but I decided to optimise the common case ---  which
> >> is that the argument is copied back and forth. Not copying the argument
> >> back is a very special case, we use it for a single compat IOCTL.
> >> 
> >> That said, we could use it for the proper ENUM_LINKS as well. Still that
> >> does not change what's normal.
> > 
> > We're talking about one comparison and one branching instruction (that
> > will not be taken in the common case). Is that micro-optimization really
> > worth it in an ioctl path that is not that performance-critical ? If you
> > think it is, could you analyse what the impact of the
> > copy_arg_to_user_nop() function on cache locality is for the common case ?
> > ;-)
> 
> I sense a certain amount of insistence in your arguments. Fine, I'll change
> it.

Thanks. I'll change that in the next version of the request API patches I will 
send out.

> You might want to send a patch removing video_device_release_empty() as
> well. :-)

Actually we should, but for an entirely different reason : most drivers that 
use video_device_release_empty() do so because they believe devm_kzalloc() is 
the best invention since sliced bread, but in reality they will crash at 
unbind time if userspace holds a reference to the video node.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space

[Sakari Ailus]

Hi Laurent,

On Sat, Jul 09, 2016 at 10:29:03PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> > Laurent Pinchart wrote:
> > > On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
> > >> Refactor copying the IOCTL argument structs from the user space and back,
> > >> in order to reduce code copied around and make the implementation more
> > >> robust.
> > >> 
> > >> As a result, the copying is done while not holding the graph mutex.
> > >> 
> > >> Signed-off-by: Sakari Ailus 
> > >> ---
> > >> since v2:
> > >> 
> > >> - Remove function to calculate maximum argument size, replace by a char
> > >>   array of 256 or kmalloc() if that's too small.
> > >>  
> > >>  drivers/media/media-device.c | 194 -
> > >>  1 file changed, 94 insertions(+), 100 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > >> index 9b5a88d..0797e4b 100644
> > >> --- a/drivers/media/media-device.c
> > >> +++ b/drivers/media/media-device.c
> 
> [snip]
> 
> > >> @@ -453,10 +432,24 @@ static long __media_device_ioctl(
> > >> 
> > >>  info = _array[_IOC_NR(cmd)];
> > >> 
> > >> +if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
> > >> +karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
> > >> +if (!karg)
> > >> +return -ENOMEM;
> > >> +}
> > >> +
> > >> +info->arg_from_user(karg, arg, cmd);
> > >> +
> > >>  mutex_lock(>graph_mutex);
> > >> -ret = info->fn(dev, arg);
> > >> +ret = info->fn(dev, karg);
> > >>  mutex_unlock(>graph_mutex);
> > >> 
> > >> +if (!ret)
> > > 
> > > How about if (!ret && info->arg_to_user) instead, and getting rid of
> > > copy_arg_to_user_nop() ?
> > 
> > I thought of that, but I decided to optimise the common case ---  which
> > is that the argument is copied back and forth. Not copying the argument
> > back is a very special case, we use it for a single compat IOCTL.
> > 
> > That said, we could use it for the proper ENUM_LINKS as well. Still that
> > does not change what's normal.
> 
> We're talking about one comparison and one branching instruction (that will 
> not be taken in the common case). Is that micro-optimization really worth it 
> in an ioctl path that is not that performance-critical ? If you think it is, 
> could you analyse what the impact of the copy_arg_to_user_nop() function on 
> cache locality is for the common case ? ;-)

I sense a certain amount of insistence in your arguments. Fine, I'll change
it.

You might want to send a patch removing video_device_release_empty() as
well. :-)

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space

[Laurent Pinchart]

Hi Sakari,

On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
> >> Refactor copying the IOCTL argument structs from the user space and back,
> >> in order to reduce code copied around and make the implementation more
> >> robust.
> >> 
> >> As a result, the copying is done while not holding the graph mutex.
> >> 
> >> Signed-off-by: Sakari Ailus 
> >> ---
> >> since v2:
> >> 
> >> - Remove function to calculate maximum argument size, replace by a char
> >>   array of 256 or kmalloc() if that's too small.
> >>  
> >>  drivers/media/media-device.c | 194 -
> >>  1 file changed, 94 insertions(+), 100 deletions(-)
> >> 
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 9b5a88d..0797e4b 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c

[snip]

> >> @@ -453,10 +432,24 @@ static long __media_device_ioctl(
> >> 
> >>info = _array[_IOC_NR(cmd)];
> >> 
> >> +  if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
> >> +  karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
> >> +  if (!karg)
> >> +  return -ENOMEM;
> >> +  }
> >> +
> >> +  info->arg_from_user(karg, arg, cmd);
> >> +
> >>mutex_lock(>graph_mutex);
> >> -  ret = info->fn(dev, arg);
> >> +  ret = info->fn(dev, karg);
> >>mutex_unlock(>graph_mutex);
> >> 
> >> +  if (!ret)
> > 
> > How about if (!ret && info->arg_to_user) instead, and getting rid of
> > copy_arg_to_user_nop() ?
> 
> I thought of that, but I decided to optimise the common case ---  which
> is that the argument is copied back and forth. Not copying the argument
> back is a very special case, we use it for a single compat IOCTL.
> 
> That said, we could use it for the proper ENUM_LINKS as well. Still that
> does not change what's normal.

We're talking about one comparison and one branching instruction (that will 
not be taken in the common case). Is that micro-optimization really worth it 
in an ioctl path that is not that performance-critical ? If you think it is, 
could you analyse what the impact of the copy_arg_to_user_nop() function on 
cache locality is for the common case ? ;-)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space

[Sakari Ailus]

Hi Laurent,

Many thanks for the review!

Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
>> Refactor copying the IOCTL argument structs from the user space and back,
>> in order to reduce code copied around and make the implementation more
>> robust.
>>
>> As a result, the copying is done while not holding the graph mutex.
>>
>> Signed-off-by: Sakari Ailus 
>> ---
>> since v2:
>>
>> - Remove function to calculate maximum argument size, replace by a char
>>   array of 256 or kmalloc() if that's too small.
>>
>>  drivers/media/media-device.c | 194 +++-
>>  1 file changed, 94 insertions(+), 100 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 9b5a88d..0797e4b 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -59,27 +59,24 @@ static int media_device_close(struct file *filp)
>>  }
>>
>>  static int media_device_get_info(struct media_device *dev,
>> - struct media_device_info __user *__info)
>> + struct media_device_info *info)
>>  {
>> -struct media_device_info info;
>> -
>> -memset(, 0, sizeof(info));
>> +memset(info, 0, sizeof(*info));
>>
>>  if (dev->driver_name[0])
>> -strlcpy(info.driver, dev->driver_name, sizeof(info.driver));
>> +strlcpy(info->driver, dev->driver_name, sizeof(info->driver));
>>  else
>> -strlcpy(info.driver, dev->dev->driver->name, 
>> sizeof(info.driver));
>> +strlcpy(info->driver, dev->dev->driver->name,
>> +sizeof(info->driver));
>>
>> -strlcpy(info.model, dev->model, sizeof(info.model));
>> -strlcpy(info.serial, dev->serial, sizeof(info.serial));
>> -strlcpy(info.bus_info, dev->bus_info, sizeof(info.bus_info));
>> +strlcpy(info->model, dev->model, sizeof(info->model));
>> +strlcpy(info->serial, dev->serial, sizeof(info->serial));
>> +strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
>>
>> -info.media_version = MEDIA_API_VERSION;
>> -info.hw_revision = dev->hw_revision;
>> -info.driver_version = dev->driver_version;
>> +info->media_version = MEDIA_API_VERSION;
>> +info->hw_revision = dev->hw_revision;
>> +info->driver_version = dev->driver_version;
>>
>> -if (copy_to_user(__info, , sizeof(*__info)))
>> -return -EFAULT;
>>  return 0;
>>  }
>>
>> @@ -101,29 +98,25 @@ static struct media_entity *find_entity(struct
>> media_device *mdev, u32 id) }
>>
>>  static long media_device_enum_entities(struct media_device *mdev,
>> -   struct media_entity_desc __user *uent)
>> +   struct media_entity_desc *entd)
>>  {
>>  struct media_entity *ent;
>> -struct media_entity_desc u_ent;
>> -
>> -memset(_ent, 0, sizeof(u_ent));
>> -if (copy_from_user(_ent.id, >id, sizeof(u_ent.id)))
>> -return -EFAULT;
>> -
>> -ent = find_entity(mdev, u_ent.id);
>>
>> +ent = find_entity(mdev, entd->id);
>>  if (ent == NULL)
>>  return -EINVAL;
>>
>> -u_ent.id = media_entity_id(ent);
>> +memset(entd, 0, sizeof(*entd));
>> +
>> +entd->id = media_entity_id(ent);
>>  if (ent->name)
>> -strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
>> -u_ent.type = ent->function;
>> -u_ent.revision = 0; /* Unused */
>> -u_ent.flags = ent->flags;
>> -u_ent.group_id = 0; /* Unused */
>> -u_ent.pads = ent->num_pads;
>> -u_ent.links = ent->num_links - ent->num_backlinks;
>> +strlcpy(entd->name, ent->name, sizeof(entd->name));
>> +entd->type = ent->function;
>> +entd->revision = 0; /* Unused */
>> +entd->flags = ent->flags;
>> +entd->group_id = 0; /* Unused */
>> +entd->pads = ent->num_pads;
>> +entd->links = ent->num_links - ent->num_backlinks;
>>
>>  /*
>>   * Workaround for a bug at media-ctl <= v1.10 that makes it to
>> @@ -139,14 +132,13 @@ static long media_device_enum_entities(struct
>> media_device *mdev, if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>>  ent->function > MEDIA_ENT_T_DEVNODE_UNKNOWN) {
>>  if (is_media_entity_v4l2_subdev(ent))
>> -u_ent.type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>> +entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>>  else if (ent->function != MEDIA_ENT_F_IO_V4L)
>> -u_ent.type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>> +entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>>  }
>>
>> -memcpy(_ent.raw, >info, sizeof(ent->info));
>> -if (copy_to_user(uent, _ent, sizeof(u_ent)))
>> -return -EFAULT;
>> +memcpy(>raw, >info, sizeof(ent->info));
>> +
>>  return 0;
>>  }
>>
>> @@ 

Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
> Refactor copying the IOCTL argument structs from the user space and back,
> in order to reduce code copied around and make the implementation more
> robust.
> 
> As a result, the copying is done while not holding the graph mutex.
> 
> Signed-off-by: Sakari Ailus 
> ---
> since v2:
> 
> - Remove function to calculate maximum argument size, replace by a char
>   array of 256 or kmalloc() if that's too small.
> 
>  drivers/media/media-device.c | 194 +++-
>  1 file changed, 94 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 9b5a88d..0797e4b 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -59,27 +59,24 @@ static int media_device_close(struct file *filp)
>  }
> 
>  static int media_device_get_info(struct media_device *dev,
> -  struct media_device_info __user *__info)
> +  struct media_device_info *info)
>  {
> - struct media_device_info info;
> -
> - memset(, 0, sizeof(info));
> + memset(info, 0, sizeof(*info));
> 
>   if (dev->driver_name[0])
> - strlcpy(info.driver, dev->driver_name, sizeof(info.driver));
> + strlcpy(info->driver, dev->driver_name, sizeof(info->driver));
>   else
> - strlcpy(info.driver, dev->dev->driver->name, 
> sizeof(info.driver));
> + strlcpy(info->driver, dev->dev->driver->name,
> + sizeof(info->driver));
> 
> - strlcpy(info.model, dev->model, sizeof(info.model));
> - strlcpy(info.serial, dev->serial, sizeof(info.serial));
> - strlcpy(info.bus_info, dev->bus_info, sizeof(info.bus_info));
> + strlcpy(info->model, dev->model, sizeof(info->model));
> + strlcpy(info->serial, dev->serial, sizeof(info->serial));
> + strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
> 
> - info.media_version = MEDIA_API_VERSION;
> - info.hw_revision = dev->hw_revision;
> - info.driver_version = dev->driver_version;
> + info->media_version = MEDIA_API_VERSION;
> + info->hw_revision = dev->hw_revision;
> + info->driver_version = dev->driver_version;
> 
> - if (copy_to_user(__info, , sizeof(*__info)))
> - return -EFAULT;
>   return 0;
>  }
> 
> @@ -101,29 +98,25 @@ static struct media_entity *find_entity(struct
> media_device *mdev, u32 id) }
> 
>  static long media_device_enum_entities(struct media_device *mdev,
> -struct media_entity_desc __user *uent)
> +struct media_entity_desc *entd)
>  {
>   struct media_entity *ent;
> - struct media_entity_desc u_ent;
> -
> - memset(_ent, 0, sizeof(u_ent));
> - if (copy_from_user(_ent.id, >id, sizeof(u_ent.id)))
> - return -EFAULT;
> -
> - ent = find_entity(mdev, u_ent.id);
> 
> + ent = find_entity(mdev, entd->id);
>   if (ent == NULL)
>   return -EINVAL;
> 
> - u_ent.id = media_entity_id(ent);
> + memset(entd, 0, sizeof(*entd));
> +
> + entd->id = media_entity_id(ent);
>   if (ent->name)
> - strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
> - u_ent.type = ent->function;
> - u_ent.revision = 0; /* Unused */
> - u_ent.flags = ent->flags;
> - u_ent.group_id = 0; /* Unused */
> - u_ent.pads = ent->num_pads;
> - u_ent.links = ent->num_links - ent->num_backlinks;
> + strlcpy(entd->name, ent->name, sizeof(entd->name));
> + entd->type = ent->function;
> + entd->revision = 0; /* Unused */
> + entd->flags = ent->flags;
> + entd->group_id = 0; /* Unused */
> + entd->pads = ent->num_pads;
> + entd->links = ent->num_links - ent->num_backlinks;
> 
>   /*
>* Workaround for a bug at media-ctl <= v1.10 that makes it to
> @@ -139,14 +132,13 @@ static long media_device_enum_entities(struct
> media_device *mdev, if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>   ent->function > MEDIA_ENT_T_DEVNODE_UNKNOWN) {
>   if (is_media_entity_v4l2_subdev(ent))
> - u_ent.type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> + entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>   else if (ent->function != MEDIA_ENT_F_IO_V4L)
> - u_ent.type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> + entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>   }
> 
> - memcpy(_ent.raw, >info, sizeof(ent->info));
> - if (copy_to_user(uent, _ent, sizeof(u_ent)))
> - return -EFAULT;
> + memcpy(>raw, >info, sizeof(ent->info));
> +
>   return 0;
>  }
> 
> @@ -158,8 +150,8 @@ static void media_device_kpad_to_upad(const struct
> media_pad *kpad, upad->flags = kpad->flags;
>