On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> wrote: > 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@xxxxxxx> wrote: >>> On Sun, 26 Aug 2012 19:59:40 -0300 >>> Ezequiel Garcia <elezegarcia@xxxxxxxxx> 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. > So, you want me to resend? And this whole patchset patchset should be dropped because we'll return something from vb2_queue_init. Thanks, Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
