Re: [PATCH 9/9] videobuf2-core: Change vb2_queue_init return type to void

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

Em 28-08-2012 14:23, Ezequiel Garcia escreveu:
> Hi Jon,
> 
> Thanks for your answers, I really appreciate it.
> 
> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote:
>> On Sun, 26 Aug 2012 19:59:40 -0300
>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>>
>>> 1.
>>> Why do we need to check for all these conditions in the first place?
>>> There are many other functions relying on "struct vb2_queue *q"
>>> not being null (almost all of them) and we don't check for it.
>>> What makes vb2_queue_init() so special that we need to check for it?
>>
>> There are plenty of developers who would argue for the removal of the
>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly
>> thereafter.  I'm a bit less convinced; there are attackers who are very
>> good at exploiting null pointer dereferences, and some systems still allow
>> the low part of the address space to be mapped.
>>
>> In general, IMO, checks for consistency make sense; it's nice if the
>> kernel can *tell* you that something is wrong.
>>
>> What's a mistake is the BUG_ON; that should really only be used in places
>> where things simply cannot continue.  In this case, the initialization can
>> be failed, the V4L2 device will likely be unavailable, but everything else
>> can continue as normal.  -EINVAL is the right response here.
>>
> 
> I see your point.
> 
> What I really can't seem to understand is why we should have a check
> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one.

Those BUG_ON() checks are there since likely the first version of VB1.
VB2 just inherited it.

The think is that letting the VB code to run without checking for some
conditions is evil, as it could cause mass memory corruption, as the
videobuf code writes on a large amount of memory (typically, something
like 512MB written on every 1/30s). So, the code has protections, in order
to try avoiding it. Even so, with VB1, when the output buffer is at the
video adapter memory region (what is called PCI2PCI memory transfers),
there are known bugs with some chipsets that will cause mass data corruption
at the hard disks (as the PCI2PCI transfers interfere at the data transfers
from/to the disk, due to hardware bugs).

Calling WARN_ON_ONCE() and returning some error code works, provided that
we enforce that the error code will be handled at the drivers that call
vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull))
and double_checking the code at VB2 callers.

Regards,
Mauro