| Message ID | 201105231558.13084.hselasky@c2i.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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. Good catch, but I think _IOWR would be right, because the result gets copied from kernelspace to userspace. It would be nice if you could send future patches inline rather than attached. I'd suggest using git format-patch and git send-email. Regards, Andreas > Signed-off-by: Hans Petter Selasky <hselasky@c2i.net> > --- > drivers/media/dvb/dvb-core/dvb_frontend.c | 5 +++-- > include/linux/dvb/frontend.h | 3 ++- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c > index 31e2c0d..d93c1ec 100644 > --- a/drivers/media/dvb/dvb-core/dvb_frontend.c > +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c > @@ -1507,7 +1507,8 @@ static int dvb_frontend_ioctl(struct file *file, > if (down_interruptible (&fepriv->sem)) > return -ERESTARTSYS; > > - if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)) > + if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY) || > + (cmd == FE_GET_PROPERTY_OLD)) > err = dvb_frontend_ioctl_properties(file, cmd, parg); > else { > fe->dtv_property_cache.state = DTV_UNDEFINED; > @@ -1562,7 +1563,7 @@ static int dvb_frontend_ioctl_properties(struct file *file, > dprintk("%s() Property cache is full, tuning\n", __func__); > > } else > - if(cmd == FE_GET_PROPERTY) { > + if(cmd == FE_GET_PROPERTY || cmd == FE_GET_PROPERTY_OLD) { > > tvps = (struct dtv_properties __user *)parg; > > diff --git a/include/linux/dvb/frontend.h b/include/linux/dvb/frontend.h > index 493a2bf..05b38c4 100644 > --- a/include/linux/dvb/frontend.h > +++ b/include/linux/dvb/frontend.h > @@ -374,7 +374,8 @@ struct dtv_properties { > }; > > #define FE_SET_PROPERTY _IOW('o', 82, struct dtv_properties) > -#define FE_GET_PROPERTY _IOR('o', 83, struct dtv_properties) > +#define FE_GET_PROPERTY _IOW('o', 83, struct dtv_properties) > +#define FE_GET_PROPERTY_OLD _IOR('o', 83, struct dtv_properties) > > > /** > -- 1.7.1.1 -- 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
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 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? In other words the IOCTL R/W (_IOC_READ, _IOC_WRITE) bits should not reflect what the IOCTL actually does, like modifying indirect data? > > It would be nice if you could send future patches inline rather than > attached. I'd suggest using git format-patch and git send-email. I will try to have a look at that. I'm waiting for the 16 patches I've submitted today to get reviewed and committed, before I start the next batch. Thank you! --HPS -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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. Signed-off-by: Hans Petter Selasky <hselasky@c2i.net> --- drivers/media/dvb/dvb-core/dvb_frontend.c | 5 +++-- include/linux/dvb/frontend.h | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c index 31e2c0d..d93c1ec 100644 --- a/drivers/media/dvb/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c @@ -1507,7 +1507,8 @@ static int dvb_frontend_ioctl(struct file *file, if (down_interruptible (&fepriv->sem)) return -ERESTARTSYS; - if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)) + if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY) || + (cmd == FE_GET_PROPERTY_OLD)) err = dvb_frontend_ioctl_properties(file, cmd, parg); else { fe->dtv_property_cache.state = DTV_UNDEFINED; @@ -1562,7 +1563,7 @@ static int dvb_frontend_ioctl_properties(struct file *file, dprintk("%s() Property cache is full, tuning\n", __func__); } else - if(cmd == FE_GET_PROPERTY) { + if(cmd == FE_GET_PROPERTY || cmd == FE_GET_PROPERTY_OLD) { tvps = (struct dtv_properties __user *)parg; diff --git a/include/linux/dvb/frontend.h b/include/linux/dvb/frontend.h index 493a2bf..05b38c4 100644 --- a/include/linux/dvb/frontend.h +++ b/include/linux/dvb/frontend.h @@ -374,7 +374,8 @@ struct dtv_properties { }; #define FE_SET_PROPERTY _IOW('o', 82, struct dtv_properties) -#define FE_GET_PROPERTY _IOR('o', 83, struct dtv_properties) +#define FE_GET_PROPERTY _IOW('o', 83, struct dtv_properties) +#define FE_GET_PROPERTY_OLD _IOR('o', 83, struct dtv_properties) /** -- 1.7.1.1