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. > 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 -- 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
