Re: [PATCH V3 2/3] ARM: net: bpf_jit: make code generation lessdependent on struct sk_filter.

From: Xi Wang
Date: Fri Apr 26 2013 - 18:19:45 EST


Thanks for CCing. One way to clean up this would be to refactor the
bpf jit interface as:

bpf_func_t bpf_jit_compile(struct sock_filter *filter, unsigned int flen);
void bpf_jit_free(bpf_func_t bpf_func);

Then both packet and seccomp filters can share the unified interface.
Also, we don't need seccomp_filter_get_len() and other helpers.

Do you want me to rebase my patch against linux-next and see how that goes?

- xi

On Fri, Apr 26, 2013 at 6:01 PM, Daniel Borkmann <dborkman@xxxxxxxxxx> wrote:
> On 04/26/2013 10:09 PM, Andrew Morton wrote:
>>
>> On Fri, 26 Apr 2013 21:47:46 +0200 Daniel Borkmann <dborkman@xxxxxxxxxx>
>> wrote:
>>>
>>> On 04/26/2013 09:26 PM, Andrew Morton wrote:
>>>>
>>>> On Fri, 26 Apr 2013 16:04:44 +0200 Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>>>>
>>>>> On Wednesday 24 April 2013 19:27:08 Nicolas Schichan wrote:
>>>>>>
>>>>>> @@ -858,7 +858,7 @@ b_epilogue:
>>>>>> }
>>>>>>
>>>>>>
>>>>>> -void bpf_jit_compile(struct sk_filter *fp)
>>>>>> +static void __bpf_jit_compile(struct jit_ctx *out_ctx)
>>>>>> {
>>>>>> struct jit_ctx ctx;
>>>>>> unsigned tmp_idx;
>>>>>> @@ -867,11 +867,10 @@ void bpf_jit_compile(struct sk_filter *fp)
>>>>>> if (!bpf_jit_enable)
>>>>>> return;
>>>>>>
>>>>>> - memset(&ctx, 0, sizeof(ctx));
>>>>>> - ctx.skf = fp;
>>>>>> + ctx = *out_ctx;
>>>>>> ctx.ret0_fp_idx = -1;
>>>>>>
>>>>>> - ctx.offsets = kzalloc(4 * (ctx.skf->len + 1), GFP_KERNEL);
>>>>>> + ctx.offsets = kzalloc(4 * (ctx.prog_len + 1), GFP_KERNEL);
>>>>>> if (ctx.offsets == NULL)
>>>>>> return;
>>>>>>
>>>>>> @@ -921,13 +920,26 @@ void bpf_jit_compile(struct sk_filter *fp)
>>>>>> print_hex_dump(KERN_INFO, "BPF JIT code: ",
>>>>>> DUMP_PREFIX_ADDRESS, 16, 4,
>>>>>> ctx.target,
>>>>>> alloc_size, false);
>>>>>> -
>>>>>> - fp->bpf_func = (void *)ctx.target;
>>>>>> out:
>>>>>> kfree(ctx.offsets);
>>>>>> +
>>>>>> + *out_ctx = ctx;
>>>>>> return;
>>>>>
>>>>>
>>>>> This part of the patch, in combination with 79617801e "filter:
>>>>> bpf_jit_comp:
>>>>> refactor and unify BPF JIT image dump output" is now causing build
>>>>> errors
>>>>> in linux-next:
>>>>>
>>>>> arch/arm/net/bpf_jit_32.c: In function '__bpf_jit_compile':
>>>>> arch/arm/net/bpf_jit_32.c:930:16: error: 'fp' undeclared (first use in
>>>>> this function)
>>>>> bpf_jit_dump(fp->len, alloc_size, 2, ctx.target);
>>>>
>>>>
>>>> Thanks, I did this. There may be a smarter way...
>>>
>>>
>>> I think also seccomp_jit_compile() would need this change then, otherwise
>>> the build
>>> with CONFIG_SECCOMP_FILTER_JIT might break.
>>
>>
>> urgh, that tears it.
>>
>>> I can fix this up for you if not already applied. I presume it's against
>>> linux-next tree?
>>
>>
>> Yup, please send something.
>
>
> Patch is attached. However, I currently don't have an ARM toolchain at hand,
> so
> uncompiled, untested.
>
> @Nicolas, Xi (cc, ref: http://thread.gmane.org/gmane.linux.kernel/1481464):
>
> If there is someday support for other archs as well, it would be nice if we
> do not have each time duplicated seccomp_jit_compile() etc functions in each
> JIT implementation, i.e. because they do basically the same. So follow-up
> {fix,clean}up is appreciated.
>
> Also, I find it a bit weird that seccomp_filter_get_len() and some other
> _one-line_ functions from kernel/seccomp.c are not placed into the
> corresponding header file as inlines.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/