| Message ID | 20210416202404.3443623-5-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | BPF static linker: support externs | expand |
On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > Define __hidden helper macro in bpf_helpers.h, which is a short-hand for > __attribute__((visibility("hidden"))). Add libbpf support to mark BPF > subprograms marked with __hidden as static in BTF information to enforce BPF > verifier's static function validation algorithm, which takes more information > (caller's context) into account during a subprogram validation. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/bpf_helpers.h | 8 ++++++ > tools/lib/bpf/btf.c | 5 ---- > tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++++++++- > tools/lib/bpf/libbpf_internal.h | 6 +++++ > 4 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 75c7581b304c..9720dc0b4605 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -47,6 +47,14 @@ > #define __weak __attribute__((weak)) > #endif > > +/* > + * Use __hidden attribute to mark a non-static BPF subprogram effectively > + * static for BPF verifier's verification algorithm purposes, allowing more > + * extensive and permissive BPF verification process, taking into account > + * subprogram's caller context. > + */ > +#define __hidden __attribute__((visibility("hidden"))) To prevent potential external __hidden macro definition conflict, how about #ifdef __hidden #undef __hidden #define __hidden __attribute__((visibility("hidden"))) #endif > + > /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include > * any system-level headers (such as stddef.h, linux/version.h, etc), and > * commonly-used macros like NULL and KERNEL_VERSION aren't available through > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index d30e67e7e1e5..d57e13a13798 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -1605,11 +1605,6 @@ static void *btf_add_type_mem(struct btf *btf, size_t add_sz) > btf->hdr->type_len, UINT_MAX, add_sz); > } > > -static __u32 btf_type_info(int kind, int vlen, int kflag) > -{ > - return (kflag << 31) | (kind << 24) | vlen; > -} > - > static void btf_type_inc_vlen(struct btf_type *t) > { > t->info = btf_type_info(btf_kind(t), btf_vlen(t) + 1, btf_kflag(t)); > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 9cc2d45b0080..ce5558d0a61b 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -71,6 +71,7 @@ > static struct bpf_map *bpf_object__add_map(struct bpf_object *obj); > static const struct btf_type * > skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id); > +static bool prog_is_subprog(const struct bpf_object *obj, const struct bpf_program *prog); > > static int __base_pr(enum libbpf_print_level level, const char *format, > va_list args) > @@ -274,6 +275,7 @@ struct bpf_program { > bpf_program_clear_priv_t clear_priv; > > bool load; > + bool mark_btf_static; > enum bpf_prog_type type; > enum bpf_attach_type expected_attach_type; > int prog_ifindex; > @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > if (err) > return err; > > + /* if function is a global/weak symbol, but has hidden > + * visibility (or any non-default one), mark its BTF FUNC as > + * static to enable more permissive BPF verification mode with > + * more outside context available to BPF verifier > + */ > + if (GELF_ST_BIND(sym.st_info) != STB_LOCAL > + && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT) Maybe we should check GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN instead? > + prog->mark_btf_static = true; > + > nr_progs++; > obj->nr_programs = nr_progs; > > @@ -2618,7 +2629,7 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) > { > struct btf *kern_btf = obj->btf; > bool btf_mandatory, sanitize; > - int err = 0; > + int i, err = 0; > > if (!obj->btf) > return 0; > @@ -2632,6 +2643,38 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) > return 0; > } > > + /* Even though some subprogs are global/weak, user might prefer more > + * permissive BPF verification process that BPF verifier performs for > + * static functions, taking into account more context from the caller > + * functions. In such case, they need to mark such subprogs with > + * __attribute__((visibility("hidden"))) and libbpf will adjust > + * corresponding FUNC BTF type to be marked as static and trigger more > + * involved BPF verification process. > + */ > + for (i = 0; i < obj->nr_programs; i++) { > + struct bpf_program *prog = &obj->programs[i]; > + struct btf_type *t; > + const char *name; > + int j, n; > + > + if (!prog->mark_btf_static || !prog_is_subprog(obj, prog)) > + continue; > + > + n = btf__get_nr_types(obj->btf); > + for (j = 1; j <= n; j++) { > + t = btf_type_by_id(obj->btf, j); > + if (!btf_is_func(t) || btf_func_linkage(t) != BTF_FUNC_GLOBAL) > + continue; > + > + name = btf__str_by_offset(obj->btf, t->name_off); > + if (strcmp(name, prog->name) != 0) > + continue; > + > + t->info = btf_type_info(BTF_KIND_FUNC, BTF_FUNC_STATIC, 0); > + break; > + } > + } > + > sanitize = btf_needs_sanitization(obj); [...]
On Wed, Apr 21, 2021 at 10:43 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > Define __hidden helper macro in bpf_helpers.h, which is a short-hand for > > __attribute__((visibility("hidden"))). Add libbpf support to mark BPF > > subprograms marked with __hidden as static in BTF information to enforce BPF > > verifier's static function validation algorithm, which takes more information > > (caller's context) into account during a subprogram validation. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/bpf_helpers.h | 8 ++++++ > > tools/lib/bpf/btf.c | 5 ---- > > tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++++++++- > > tools/lib/bpf/libbpf_internal.h | 6 +++++ > > 4 files changed, 58 insertions(+), 6 deletions(-) > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > index 75c7581b304c..9720dc0b4605 100644 > > --- a/tools/lib/bpf/bpf_helpers.h > > +++ b/tools/lib/bpf/bpf_helpers.h > > @@ -47,6 +47,14 @@ > > #define __weak __attribute__((weak)) > > #endif > > > > +/* > > + * Use __hidden attribute to mark a non-static BPF subprogram effectively > > + * static for BPF verifier's verification algorithm purposes, allowing more > > + * extensive and permissive BPF verification process, taking into account > > + * subprogram's caller context. > > + */ > > +#define __hidden __attribute__((visibility("hidden"))) > > To prevent potential external __hidden macro definition conflict, how > about > > #ifdef __hidden > #undef __hidden > #define __hidden __attribute__((visibility("hidden"))) > #endif > We do force #undef only with __always_inline because of the bad definition in linux/stddef.h And we check #ifndef for __weak, because __weak is defined in kernel headers. This is not really the case for __hidden, the only definition is in tools/lib/traceevent/event-parse-local.h, which I don't think we should worry about in BPF context. So I wanted to keep it simple and fix only if that really causes some real conflicts. And keep in mind that in BPF code bpf_helpers.h is usually included as one of the first few headers anyways. > > + > > /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include > > * any system-level headers (such as stddef.h, linux/version.h, etc), and > > * commonly-used macros like NULL and KERNEL_VERSION aren't available through [...] > > @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > > if (err) > > return err; > > > > + /* if function is a global/weak symbol, but has hidden > > + * visibility (or any non-default one), mark its BTF FUNC as > > + * static to enable more permissive BPF verification mode with > > + * more outside context available to BPF verifier > > + */ > > + if (GELF_ST_BIND(sym.st_info) != STB_LOCAL > > + && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT) > > Maybe we should check GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN > instead? It felt like only STV_DEFAULT should be "exported", semantically speaking. Everything else would be treated as if it was static, except that C rules require that function has to be global. Do you think there is some danger to do it this way? Currently static linker doesn't do anything special for STV_INTERNAL and STV_PROTECTED, so we could just disable those. Do you prefer that? I just felt that there is no risk of regression if we do this for non-STV_DEFAULT generically. > > > + prog->mark_btf_static = true; > > + > > nr_progs++; > > obj->nr_programs = nr_progs; > > [...]
On 4/22/21 11:09 AM, Andrii Nakryiko wrote: > On Wed, Apr 21, 2021 at 10:43 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 4/16/21 1:23 PM, Andrii Nakryiko wrote: >>> Define __hidden helper macro in bpf_helpers.h, which is a short-hand for >>> __attribute__((visibility("hidden"))). Add libbpf support to mark BPF >>> subprograms marked with __hidden as static in BTF information to enforce BPF >>> verifier's static function validation algorithm, which takes more information >>> (caller's context) into account during a subprogram validation. >>> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >>> --- >>> tools/lib/bpf/bpf_helpers.h | 8 ++++++ >>> tools/lib/bpf/btf.c | 5 ---- >>> tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++++++++- >>> tools/lib/bpf/libbpf_internal.h | 6 +++++ >>> 4 files changed, 58 insertions(+), 6 deletions(-) >>> >>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >>> index 75c7581b304c..9720dc0b4605 100644 >>> --- a/tools/lib/bpf/bpf_helpers.h >>> +++ b/tools/lib/bpf/bpf_helpers.h >>> @@ -47,6 +47,14 @@ >>> #define __weak __attribute__((weak)) >>> #endif >>> >>> +/* >>> + * Use __hidden attribute to mark a non-static BPF subprogram effectively >>> + * static for BPF verifier's verification algorithm purposes, allowing more >>> + * extensive and permissive BPF verification process, taking into account >>> + * subprogram's caller context. >>> + */ >>> +#define __hidden __attribute__((visibility("hidden"))) >> >> To prevent potential external __hidden macro definition conflict, how >> about >> >> #ifdef __hidden >> #undef __hidden >> #define __hidden __attribute__((visibility("hidden"))) >> #endif >> > > We do force #undef only with __always_inline because of the bad > definition in linux/stddef.h And we check #ifndef for __weak, because > __weak is defined in kernel headers. This is not really the case for > __hidden, the only definition is in > tools/lib/traceevent/event-parse-local.h, which I don't think we > should worry about in BPF context. So I wanted to keep it simple and > fix only if that really causes some real conflicts. > > And keep in mind that in BPF code bpf_helpers.h is usually included as > one of the first few headers anyways. That is fine. Conflict of __hidden is a low risk and we can deal with it later if needed. > > >>> + >>> /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include >>> * any system-level headers (such as stddef.h, linux/version.h, etc), and >>> * commonly-used macros like NULL and KERNEL_VERSION aren't available through > > [...] > >>> @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, >>> if (err) >>> return err; >>> >>> + /* if function is a global/weak symbol, but has hidden >>> + * visibility (or any non-default one), mark its BTF FUNC as >>> + * static to enable more permissive BPF verification mode with >>> + * more outside context available to BPF verifier >>> + */ >>> + if (GELF_ST_BIND(sym.st_info) != STB_LOCAL >>> + && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT) >> >> Maybe we should check GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN >> instead? > > It felt like only STV_DEFAULT should be "exported", semantically > speaking. Everything else would be treated as if it was static, except > that C rules require that function has to be global. Do you think > there is some danger to do it this way? > > Currently static linker doesn't do anything special for STV_INTERNAL > and STV_PROTECTED, so we could just disable those. Do you prefer that? Yes, let us just deal with STV_DEFAULT and STV_HIDDEN. We already specialized STV_HIDDEN, so we should not treat STV_INTERNAL/PROTECTED as what they mean in ELF standard, so let us disable them for now. > > I just felt that there is no risk of regression if we do this for > non-STV_DEFAULT generically. > > >> >>> + prog->mark_btf_static = true; >>> + >>> nr_progs++; >>> obj->nr_programs = nr_progs; >>> > > [...] >
On Thu, Apr 22, 2021 at 4:00 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/22/21 11:09 AM, Andrii Nakryiko wrote: > > On Wed, Apr 21, 2021 at 10:43 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> > >> > >> On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > >>> Define __hidden helper macro in bpf_helpers.h, which is a short-hand for > >>> __attribute__((visibility("hidden"))). Add libbpf support to mark BPF > >>> subprograms marked with __hidden as static in BTF information to enforce BPF > >>> verifier's static function validation algorithm, which takes more information > >>> (caller's context) into account during a subprogram validation. > >>> > >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > >>> --- > >>> tools/lib/bpf/bpf_helpers.h | 8 ++++++ > >>> tools/lib/bpf/btf.c | 5 ---- > >>> tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++++++++- > >>> tools/lib/bpf/libbpf_internal.h | 6 +++++ > >>> 4 files changed, 58 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > >>> index 75c7581b304c..9720dc0b4605 100644 > >>> --- a/tools/lib/bpf/bpf_helpers.h > >>> +++ b/tools/lib/bpf/bpf_helpers.h > >>> @@ -47,6 +47,14 @@ > >>> #define __weak __attribute__((weak)) > >>> #endif > >>> > >>> +/* > >>> + * Use __hidden attribute to mark a non-static BPF subprogram effectively > >>> + * static for BPF verifier's verification algorithm purposes, allowing more > >>> + * extensive and permissive BPF verification process, taking into account > >>> + * subprogram's caller context. > >>> + */ > >>> +#define __hidden __attribute__((visibility("hidden"))) > >> > >> To prevent potential external __hidden macro definition conflict, how > >> about > >> > >> #ifdef __hidden > >> #undef __hidden > >> #define __hidden __attribute__((visibility("hidden"))) > >> #endif > >> > > > > We do force #undef only with __always_inline because of the bad > > definition in linux/stddef.h And we check #ifndef for __weak, because > > __weak is defined in kernel headers. This is not really the case for > > __hidden, the only definition is in > > tools/lib/traceevent/event-parse-local.h, which I don't think we > > should worry about in BPF context. So I wanted to keep it simple and > > fix only if that really causes some real conflicts. > > > > And keep in mind that in BPF code bpf_helpers.h is usually included as > > one of the first few headers anyways. > > That is fine. Conflict of __hidden is a low risk and we can deal with it > later if needed. > > > > > > >>> + > >>> /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include > >>> * any system-level headers (such as stddef.h, linux/version.h, etc), and > >>> * commonly-used macros like NULL and KERNEL_VERSION aren't available through > > > > [...] > > > >>> @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > >>> if (err) > >>> return err; > >>> > >>> + /* if function is a global/weak symbol, but has hidden > >>> + * visibility (or any non-default one), mark its BTF FUNC as > >>> + * static to enable more permissive BPF verification mode with > >>> + * more outside context available to BPF verifier > >>> + */ > >>> + if (GELF_ST_BIND(sym.st_info) != STB_LOCAL > >>> + && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT) > >> > >> Maybe we should check GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN > >> instead? > > > > It felt like only STV_DEFAULT should be "exported", semantically > > speaking. Everything else would be treated as if it was static, except > > that C rules require that function has to be global. Do you think > > there is some danger to do it this way? > > > > Currently static linker doesn't do anything special for STV_INTERNAL > > and STV_PROTECTED, so we could just disable those. Do you prefer that? > > Yes, let us just deal with STV_DEFAULT and STV_HIDDEN. We already > specialized STV_HIDDEN, so we should not treat STV_INTERNAL/PROTECTED > as what they mean in ELF standard, so let us disable them for now. Yep, will do > > > > > I just felt that there is no risk of regression if we do this for > > non-STV_DEFAULT generically. > > > > > >> > >>> + prog->mark_btf_static = true; > >>> + > >>> nr_progs++; > >>> obj->nr_programs = nr_progs; > >>> > > > > [...] > >
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 75c7581b304c..9720dc0b4605 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -47,6 +47,14 @@ #define __weak __attribute__((weak)) #endif +/* + * Use __hidden attribute to mark a non-static BPF subprogram effectively + * static for BPF verifier's verification algorithm purposes, allowing more + * extensive and permissive BPF verification process, taking into account + * subprogram's caller context. + */ +#define __hidden __attribute__((visibility("hidden"))) + /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include * any system-level headers (such as stddef.h, linux/version.h, etc), and * commonly-used macros like NULL and KERNEL_VERSION aren't available through diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index d30e67e7e1e5..d57e13a13798 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -1605,11 +1605,6 @@ static void *btf_add_type_mem(struct btf *btf, size_t add_sz) btf->hdr->type_len, UINT_MAX, add_sz); } -static __u32 btf_type_info(int kind, int vlen, int kflag) -{ - return (kflag << 31) | (kind << 24) | vlen; -} - static void btf_type_inc_vlen(struct btf_type *t) { t->info = btf_type_info(btf_kind(t), btf_vlen(t) + 1, btf_kflag(t)); diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 9cc2d45b0080..ce5558d0a61b 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -71,6 +71,7 @@ static struct bpf_map *bpf_object__add_map(struct bpf_object *obj); static const struct btf_type * skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id); +static bool prog_is_subprog(const struct bpf_object *obj, const struct bpf_program *prog); static int __base_pr(enum libbpf_print_level level, const char *format, va_list args) @@ -274,6 +275,7 @@ struct bpf_program { bpf_program_clear_priv_t clear_priv; bool load; + bool mark_btf_static; enum bpf_prog_type type; enum bpf_attach_type expected_attach_type; int prog_ifindex; @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, if (err) return err; + /* if function is a global/weak symbol, but has hidden + * visibility (or any non-default one), mark its BTF FUNC as + * static to enable more permissive BPF verification mode with + * more outside context available to BPF verifier + */ + if (GELF_ST_BIND(sym.st_info) != STB_LOCAL + && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT) + prog->mark_btf_static = true; + nr_progs++; obj->nr_programs = nr_progs; @@ -2618,7 +2629,7 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) { struct btf *kern_btf = obj->btf; bool btf_mandatory, sanitize; - int err = 0; + int i, err = 0; if (!obj->btf) return 0; @@ -2632,6 +2643,38 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) return 0; } + /* Even though some subprogs are global/weak, user might prefer more + * permissive BPF verification process that BPF verifier performs for + * static functions, taking into account more context from the caller + * functions. In such case, they need to mark such subprogs with + * __attribute__((visibility("hidden"))) and libbpf will adjust + * corresponding FUNC BTF type to be marked as static and trigger more + * involved BPF verification process. + */ + for (i = 0; i < obj->nr_programs; i++) { + struct bpf_program *prog = &obj->programs[i]; + struct btf_type *t; + const char *name; + int j, n; + + if (!prog->mark_btf_static || !prog_is_subprog(obj, prog)) + continue; + + n = btf__get_nr_types(obj->btf); + for (j = 1; j <= n; j++) { + t = btf_type_by_id(obj->btf, j); + if (!btf_is_func(t) || btf_func_linkage(t) != BTF_FUNC_GLOBAL) + continue; + + name = btf__str_by_offset(obj->btf, t->name_off); + if (strcmp(name, prog->name) != 0) + continue; + + t->info = btf_type_info(BTF_KIND_FUNC, BTF_FUNC_STATIC, 0); + break; + } + } + sanitize = btf_needs_sanitization(obj); if (sanitize) { const void *raw_data; diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index 6017902c687e..92b7eae10c6d 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -19,6 +19,7 @@ #pragma GCC poison reallocarray #include "libbpf.h" +#include "btf.h" #ifndef EM_BPF #define EM_BPF 247 @@ -132,6 +133,11 @@ struct btf_type; struct btf_type *btf_type_by_id(struct btf *btf, __u32 type_id); +static inline __u32 btf_type_info(int kind, int vlen, int kflag) +{ + return (kflag << 31) | (kind << 24) | vlen; +} + void *libbpf_add_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t cur_cnt, size_t max_cnt, size_t add_cnt); int libbpf_ensure_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t need_cnt);
Define __hidden helper macro in bpf_helpers.h, which is a short-hand for __attribute__((visibility("hidden"))). Add libbpf support to mark BPF subprograms marked with __hidden as static in BTF information to enforce BPF verifier's static function validation algorithm, which takes more information (caller's context) into account during a subprogram validation. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/bpf_helpers.h | 8 ++++++ tools/lib/bpf/btf.c | 5 ---- tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++++++++- tools/lib/bpf/libbpf_internal.h | 6 +++++ 4 files changed, 58 insertions(+), 6 deletions(-)