On Sat, Jun 30, 2012 at 10:54:25AM +0200, Florian Westphal wrote: > 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. OK, fair enough. I've enqueued this to nf-next. -- 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
