Re: [PATCH v2] filter: Optimize instruction revalidation code.

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mardi 16 novembre 2010 à 23:31 +0900, Tetsuo Handa a écrit :
> Eric Dumazet wrote:
> > Patch seems fine to me, with the 'const' codes[] Michael Tokarev already
> > spotted.
> 
> Sorry, I forgot to evaluate
> 
> 	default:
> 		return -EINVAL;
> 	}
> 
> case. If caller passed ftest->code == BPF_S_ALU_DIV_K (translated value)
> rather than ftest->code == BPF_ALU|BPF_DIV|BPF_K (original value), the check
> 
> 	case BPF_ALU|BPF_DIV|BPF_K:
> 		if (ftest->k == 0)
> 			return -EINVAL;
> 		ftest->code = BPF_S_ALU_DIV_K;
> 		break;
> 
> is bypassed. The problem is that original value and translated value overwraps.
> Can we change translated value in order to guarantee that these values never
> overwraps?


I dont understand the problem...
Once translated, you have to test the translated code, not the original
one ;)


> ----------------------------------------
>  From 7b6a7b784fa096383aadc86d32bff6b8329a2e66 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 16 Nov 2010 22:37:45 +0900
> Subject: [PATCH v2] filter: optimize instruction revalidation code.
> 
> Since repeating small value to small value conversion using switch() clause's
> case statement is wasteful, this patch instroduces u16 to u16 mapping table
> and removes most of case statements. As a result, the size of net/core/filter.o
> is reduced by about 27% on x86.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  net/core/filter.c |  223 ++++++++++++++++-------------------------------------
>  1 files changed, 68 insertions(+), 155 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 23e9b2a..ef1d226 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -383,7 +383,57 @@ EXPORT_SYMBOL(sk_run_filter);
>   */
>  int sk_chk_filter(struct sock_filter *filter, int flen)
>  {
> -	struct sock_filter *ftest;
> +	/*
> +	 * Valid instructions are initialized to non-0.
> +	 * Invalid instructions are initialized to 0.
> +	 */
> +	static const u16 codes[] = {


why u16 ? 

You store translated instructions, so u8 is OK

> +		[BPF_ALU|BPF_ADD|BPF_K]  = BPF_S_ALU_ADD_K + 1,
> +		[BPF_ALU|BPF_ADD|BPF_X]  = BPF_S_ALU_ADD_X + 1,
> +		[BPF_ALU|BPF_SUB|BPF_K]  = BPF_S_ALU_SUB_K + 1,
> +		[BPF_ALU|BPF_SUB|BPF_X]  = BPF_S_ALU_SUB_X + 1,
> +		[BPF_ALU|BPF_MUL|BPF_K]  = BPF_S_ALU_MUL_K + 1,



Also fix the indentation at the end of sk_chk_filter()

You have 3 extra tabulations :

        /* last instruction must be a RET code */
        switch (filter[flen - 1].code) {
        case BPF_S_RET_K:
        case BPF_S_RET_A:
                return 0;
                break;
!here!          default:
!here!                   return -EINVAL;
!here!          }
}
EXPORT_SYMBOL(sk_chk_filter);