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. Regards, Mauro -- 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
