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

[Jonathan Corbet <corbet@lwn.net>]

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.

> 2.
> If a DoS attack is the concern here, I wonder how this would be achieved?
> vb2_queue_init() is an "internal" (so to speak) function, that will only
> be called by videobuf2 drivers.

It would depend on a driver bug, but the sad fact is that driver bugs do
exist.  Perhaps it's as simple as getting the driver module to load when
the hardware is absent, for example.

> I'm not arguing, truly. I wan't to understand what's the rationale behind
> putting BUG_ON, or WARN_ON, or return -EINVAL in a case like this.

In short: we want the kernel to be as robust as it can be.  Detecting
problems before they can snowball helps in that regard.  Hitting the big
red BUG_ON() button in situations where things can continue does not.  At
least, that's how I see it.

jon