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.

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.

[Andreas Oberritter]

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.

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.

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.

Regards,
Andreas

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]

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

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.

[Andreas Oberritter]

On 06/01/2011 11:15 PM, Mauro Carvalho Chehab wrote:
> The dvb_usercopy will do the right thing, if we use _IOR or _IORW.

It only works, because _IOC_READ triggers a copy_from_user, as a
workaround for wrongly marked ioctls like this, according to a code
comment. It does not really do the right thing, because in this special
case the later call to copy_to_user isn't required. But it doesn't do
any real harm either.

> 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.

Either way is fine for me.

Regards,
Andreas

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]

Em 03-06-2011 09:44, Andreas Oberritter escreveu:
> On 06/01/2011 11:15 PM, Mauro Carvalho Chehab wrote:
>> The dvb_usercopy will do the right thing, if we use _IOR or _IORW.
> 
> It only works, because _IOC_READ triggers a copy_from_user, as a
> workaround for wrongly marked ioctls like this, according to a code
> comment. It does not really do the right thing, because in this special
> case the later call to copy_to_user isn't required. But it doesn't do
> any real harm either.

Yes, that's what I meant to say ;) The workaround for it is there already,
so maybe there are other ioctl's using the wrong _IOC_ directions.

As I said before, some ioctl's don't use _IOC_ directions, like for example
the tty ioctls like TIO* ones. This happens on several very old drivers.
So, ioctl core don't make any assumption about them. it is up to each driver
(or subsystem core) to handle it.

>> 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.
> 
> Either way is fine for me.

I'm not against fixing it, but, in this case, we'll need to validate all DVB
ioctl's and remove the IOC_READ hack for all non-_OLD controls, and writing
a notice at features-to-be-removed announcing that the _OLD controls will be
removed.

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