Re: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not broken.

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 23-05-2011 12:32, Andreas Oberritter escreveu:
> On 05/23/2011 04:51 PM, Hans Petter Selasky wrote:
>> On Monday 23 May 2011 16:37:18 Andreas Oberritter wrote:
>>> On 05/23/2011 03:58 PM, Hans Petter Selasky wrote:
>>>> From be7d0f72ebf4d945cfb2a5c9cc871707f72e1e3c Mon Sep 17 00:00:00 2001
>>>> From: Hans Petter Selasky <hselasky@c2i.net>
>>>> Date: Mon, 23 May 2011 15:56:31 +0200
>>>> Subject: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated
>>>> structure is transferred from userspace to kernelspace. Keep the old
>>>> ioctl around for compatibility so that existing code is not broken.
>>>
>>
>> Hi,
>>
>>> Good catch, but I think _IOWR would be right, because the result gets
>>> copied from kernelspace to userspace.
>>
>> Those flags are only for the IOCTL associated structure itself. The V4L DVB 
>> kernel only reads the dtv_properties structure in either case and does not 
>> write any data back to it. That's why only _IOW is required.
> 
> I see.
> 
>> I checked somewhat and the R/W bits in the IOCTL command does not appear do be 
>> matched to the R/W permissions you have on the file handle? Or am I mistaken?
> 
> You're right. There's no direct relationship between them, at least not
> within dvb-core.
> 
>> In other words the IOCTL R/W (_IOC_READ, _IOC_WRITE) bits should not reflect 
>> what the IOCTL actually does, like modifying indirect data?
> 
> I'm not sure. Your patch is certainly doing the right thing for the
> current implementation of dvb_usercopy, which however wasn't designed
> with variable length arrays in mind.

The dvb_usercopy will do the right thing, if we use _IOR or _IORW.

> Taking dvb_usercopy aside, my interpretation of the ioctl bits was:
> - _IOC_READ is required if copy_to_user/put_user needs to be used during
> the ioctl.
> - _IOC_WRITE is required if copy_from_user/get_user needs to be used
> during the ioctl.

That is my understanding too. I agree that _IOWR seems to be the more appropriate
definition for it.

That's said, this is just a naming convention. Kernel core won't enforce
any special behavior, as there are some violations about this convention
on a few places.

> 
> Whether that's limited to the structure directly encoded in the ioctl or
> not is unclear to me. Maybe someone at LKML can shed some light on that.

I prefer to not apply this patch, as it won't fix anything. Adding an _OLD means
that we'll need later to remove it, causing a regression. Ok, we may do like we did
with V4L _OLD ioctl's that were marked as _OLD at 2.6.5 and were removed on a late
2.6.3x.

Cheers,
Mauro