From: Patrick McHardy > Kris Katterjohn wrote: > > From: Patrick McHardy > > > >>Kris Katterjohn wrote: > >> > >>>--- x/net/core/filter.c 2005-12-28 16:51:35.000000000 -0600 > >>>+++ y/net/core/filter.c 2005-12-28 16:53:32.000000000 -0600 > >>>@@ -250,7 +250,7 @@ load_b: > >>> mem[fentry->k] = X; > >>> continue; > >>> default: > >>>- /* Invalid instruction counts as RET */ > >>>+ /* Should never be reached */ > >> > >>This stuff has had a number of bad bugs before, if it can't be reached, > >>please call BUG or something similar instead of silently ignoring the > >>error. > > > > > > I'll just remove the default label and return statement. > > 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? > > The runtime is for BPF_ALU|BPF_DIV|BPF_X, not BPF_ALU|BPF_DIV|BPF_K. The > > BPF_K > > instruction is constant and can be checked for at any time, but with BPF_X, > > it changes with each packet and must be checked at runtime. > > Ah, I missed that you already moved the check for div-by-constant > in a previous patch. So the entire point of this patch is to remove > the default-branch in the switch-statement? That hardly seems worth > the additional amount of code. 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. 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. - 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
