linux-kernel - Re: [RFC] vtunerc: virtual DVB device - is it ok to NACK driver because of worrying about possible misusage?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJbz7-2mQ-mCYyqGR8iuP-z_ZaDaGtBS9V9oQxk9pd99tA7AEg@mail.gmail.com>
Date:	Tue, 6 Dec 2011 18:35:50 +0100
From:	HoP <jpetrous@...il.com>
To:	Andreas Oberritter <obi@...uxtv.org>
Cc:	Mauro Carvalho Chehab <mchehab@...hat.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] vtunerc: virtual DVB device - is it ok to NACK driver
 because of worrying about possible misusage?

Hi Andreas

[...]

>>> You don't need to wait for write-only operations. Basically all demux
>>> ioctls are write-only. Since vtunerc is using dvb-core's software demux
>>> *locally*, errors for invalid arguments etc. will be returned as usual.
>>>
>>> What's left is one call to FE_SET_FRONTEND for each frequency to tune
>>> to, and one FE_READ_STATUS for each time the lock status is queried.
>>> Note that one may use FE_GET_EVENT instead of FE_READ_STATUS to get
>>> notified of status changes asynchronously if desired.
>>>
>>> Btw.: FE_SET_FRONTEND doesn't block either, because the driver callback
>>> is called from a dvb_frontend's *local* kernel thread.
>>
>> Still, vtunerc waits for write operations:
>>
>> http://code.google.com/p/vtuner/source/browse/vtunerc_proxyfe.c?repo=linux-driver#285
>>
>> No matter if they are read or write, all of them call this function:
>>
>> http://code.google.com/p/vtuner/source/browse/vtunerc_ctrldev.c?repo=linux-driver#390
>>
>> That has a wait_event inside that function, as everything is directed to
>> the userspace.
>
> Please, stop writing such bullshit! Just before the call to wait_event
> there's code to return from the function if waiting has not been requested.
>
>> This is probably the way Florian found to return the errors returned by
>> the ioctls. This driver is synchronous, with simplifies it, at the lack of
>> performance.
>
> The fix is easy: set the third parameter to 0. A DVB application doesn't
> need to know whether SET_VOLTAGE etc. succeeded or not, because it won't
> get any feedback from the switch. If tuning fails, it has to retry
> anyway, even if all ioctls returned 0.
>
>> Ok, the driver could be smarter than that, and some heuristics could be
>> added into it, in order to foresee the likely error code, returning it
>> in advance, and then implementing some asynchronous mechanism that would
>> handle the error later, but that would be complex and may still introduce
>> some bad behaviors.
>
> There's no need to handle errors that don't occur.
>
> Nobody said that the implementation of vtunerc was perfect. I've already
> listed many things that need to be changed in order to even consider it
> for merging.

Exactly that was an intention for my first RFC - to get driver reviewed by
kernel hackers and enhance/fixe it to prepare it for merging.

I'm going to address all your findings during Xmass, so hopefully I will
spam here again after New Year :-)

I very appreciate your POV which helped me stay remembering
it is not way to the wall.

Thanks

Honza
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/