Jan Engelhardt <jengelh@xxxxxxx> wrote: > On Friday 2012-06-29 13:15, Pablo Neira Ayuso wrote: > >On Thu, Jun 28, 2012 at 03:57:47PM +0300, Tomasz Bursztyka wrote: > >> --- > >> @@ -184,9 +184,11 @@ replay: > >> lockdep_is_held(&nfnl_mutex)) != ss || > >> nfnetlink_find_client(type, ss) != nc) > >> err = -EAGAIN; > >> - else > >> + else if (nc->call) > >> err = nc->call(net->nfnl, skb, nlh, > >> (const struct nlattr **)cda); > >> + else > >> + err = -EINVAL; > > > >Hm, I think this is redundant. We got the ipset problem fixed with > >2/3. I think we can add some BUG_ON instead as all nc->call needs to > >be defined. Otherwise, spot a bug message so it is fixed. > > Seriously, we have run into NULL dereferences at this place so often > that it should not be discarded as a PEBKAC issue. Yes, it is the > programmer's responsibility to fill in .call, but error handling is > anytime better than driving one's machine into the pit stop due to a > NULL deref or BUG_ON. Agreed. I think this should be WARN_ON_ONCE + ret EINVAL. We also do this with various places that e.g. fill netlink messages so that adding new attributes without bumping allocation sizes is reported without crashes. Since we've started to move the performance-critical nfnetlink stuff over to ->call_rcu the extra NULL test should not harm fastpath either. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
