Re: [PATCH 2/3] bpfilter: include bpfilter_umh in assembly instead of using objcopy

[Masahiro Yamada <yamada.masahiro@socionext.com>]

2018-06-09 5:47 GMT+09:00 Alexei Starovoitov <alexei.starovoitov@gmail.com>:
> On Sat, Jun 09, 2018 at 02:12:09AM +0900, Masahiro Yamada wrote:
>> Do not use the troublesome ELF magic.  What is happening here is to
>> embed a user-space program into the kernel.  Simply wrap it in the
>> assembly with the '.incbin' directive.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  net/bpfilter/Makefile            | 15 ++-------------
>>  net/bpfilter/bpfilter_kern.c     | 11 +++++------
>>  net/bpfilter/bpfilter_umh_blob.S |  7 +++++++
>>  3 files changed, 14 insertions(+), 19 deletions(-)
>>  create mode 100644 net/bpfilter/bpfilter_umh_blob.S
>>
>> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
>> index aafa720..39c6980 100644
>> --- a/net/bpfilter/Makefile
>> +++ b/net/bpfilter/Makefile
>> @@ -15,18 +15,7 @@ ifeq ($(CONFIG_BPFILTER_UMH), y)
>>  HOSTLDFLAGS += -static
>>  endif
>>
>> -# a bit of elf magic to convert bpfilter_umh binary into a binary blob
>> -# inside bpfilter_umh.o elf file referenced by
>> -# _binary_net_bpfilter_bpfilter_umh_start symbol
>> -# which bpfilter_kern.c passes further into umh blob loader at run-time
>> -quiet_cmd_copy_umh = GEN $@
>> -      cmd_copy_umh = echo ':' > $(obj)/.bpfilter_umh.o.cmd; \
>> -      $(OBJCOPY) -I binary -O $(CONFIG_OUTPUT_FORMAT) \
>> -      -B `$(OBJDUMP) -f $<|grep architecture|cut -d, -f1|cut -d' ' -f2` \
>> -      --rename-section .data=.init.rodata $< $@
>> -
>> -$(obj)/bpfilter_umh.o: $(obj)/bpfilter_umh
>> -     $(call cmd,copy_umh)
>> +$(obj)/bpfilter_umh_blob.o: $(obj)/bpfilter_umh
>>
>>  obj-$(CONFIG_BPFILTER_UMH) += bpfilter.o
>> -bpfilter-objs += bpfilter_kern.o bpfilter_umh.o
>> +bpfilter-objs += bpfilter_kern.o bpfilter_umh_blob.o
>> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
>> index b13d058..fcc1a7c 100644
>> --- a/net/bpfilter/bpfilter_kern.c
>> +++ b/net/bpfilter/bpfilter_kern.c
>> @@ -10,11 +10,8 @@
>>  #include <linux/file.h>
>>  #include "msgfmt.h"
>>
>> -#define UMH_start _binary_net_bpfilter_bpfilter_umh_start
>> -#define UMH_end _binary_net_bpfilter_bpfilter_umh_end
>> -
>> -extern char UMH_start;
>> -extern char UMH_end;
>> +extern char bpfilter_umh_start;
>> +extern char bpfilter_umh_end;
>>
>>  static struct umh_info info;
>>  /* since ip_getsockopt() can run in parallel, serialize access to umh */
>> @@ -89,7 +86,9 @@ static int __init load_umh(void)
>>       int err;
>>
>>       /* fork usermode process */
>> -     err = fork_usermode_blob(&UMH_start, &UMH_end - &UMH_start, &info);
>> +     err = fork_usermode_blob(&bpfilter_umh_end,
>> +                              &bpfilter_umh_end - &bpfilter_umh_start,
>> +                              &info);
>>       if (err)
>>               return err;
>>       pr_info("Loaded bpfilter_umh pid %d\n", info.pid);
>> diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S
>> new file mode 100644
>> index 0000000..40311d1
>> --- /dev/null
>> +++ b/net/bpfilter/bpfilter_umh_blob.S
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +     .section .init.rodata, "a"
>> +     .global bpfilter_umh_start
>> +bpfilter_umh_start:
>> +     .incbin "net/bpfilter/bpfilter_umh"
>
> Interesting. I think this is good idea. Looks cleaner than objcopy magic.
> btw CONFIG_OUTPUT_FORMAT already fixed by
> commit 8d97ca6b6755 ("bpfilter: fix OUTPUT_FORMAT") in net tree.
> Could you please rebase on top of that tree?
>


OK, I will rebase it.

BTW, I only compile-tested this patch.
Could you check if it really works?



-- 
Best Regards
Masahiro Yamada