From: Patrick McHardy > Kris Katterjohn wrote: > > From: Patrick McHardy > > > >>Thats even worse, it will fall through to the second switch statement. > >>Just call BUG(), since it means you forgot to check something. > > > > > > I'm 99.99% sure I didn't forget anything. I just left it there in my first > > patch > > because it was there before. Everything is checked for in sk_chk_filter(), > > so > > the default branch will never be used. And if you think we need the default > > branch, why call BUG instead of just returning 0 as before? > > Because it means a check was missing. We had a number of bad bugs > in there before, better to find them quick than have them lurking > around until someone does an code audit. But in this case I tend > to agree with you, it doesn't seem to affect anything critical.
So keep the return statement, then? > > No. The point of this patch is to stop a bad filter instruction from > > causing the > > kernel to loop in sk_run_filter(), but always return 0. If we check to make > > sure > > only valid instructions are used in sk_chk_filter(), then we eliminate that > > problem. > > That seems to be a purely theoretical problem, the common case is > correctly compiled filters from libpcap. BTW: I haven't read the > BPF specs/paper/whatever, but if this is documented behaviour, > something might actually be relying on it. I don't use libpcap much anymore (not because it's bad or anything) and I've seen many programs that don't and just use custom BPF filters. I think it's a definite plus to check these things in sk_chk_filter(). The only thing I've read over BPF is that USENIX paper from (I think) 1993 and I don't recall it saying anything about the actual implementation of it, just going over the instruction set. See below, though. > My idea is to check for everything in sk_chk_filter(). Check everything > > there and make sure that when sk_run_filter() is called, it shouldn't have > > any > > problems. I moved the check for constant-division-by-zero to > > sk_chk_filter() because it could be checked there. Now, I think a check for > > only valid > > instructions should be there too, because we can go ahead and check it > > there. > > sk_chk_filter() should return an error if the filter is bad, not allow it > > and > > have sk_run_filter() give you nothing. > > Well, I'm not convinced that there is a problem, but I do agree that its > good to return an error for invalid filters - if they are actually > invalid and its not documented behaviour to accept unknown instructions > and treat them as return. After some googling, I found this OpenBSD kernel code which does a lot of what my patch would add, but uses BPF_CLASS as you suggested earlier: http://fxr.watson.org/fxr/source//net/bpf_filter.c?v=OPENBSD My guess would be there isn't any requirement to return for a bad instruction while running the filter. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
