Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1341156 > unrolled thread

Re: [RFC v2 2/7] tables.h: add linker table support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC v2 2/7] tables.h: add linker table support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-02-24 00:10 +0100
    Re: [RFC v2 2/7] tables.h: add linker table support "H. Peter Anvin" <hpa@zytor.com> - 2016-02-24 00:30 +0100
      Re: [RFC v2 2/7] tables.h: add linker table support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-02-24 00:40 +0100
        Re: [RFC v2 2/7] tables.h: add linker table support "H. Peter Anvin" <hpa@zytor.com> - 2016-02-24 01:20 +0100
          Re: [RFC v2 2/7] tables.h: add linker table support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-02-24 02:00 +0100

#1341156 — Re: [RFC v2 2/7] tables.h: add linker table support

On Fri, Feb 19, 2016 at 1:48 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Fri, Feb 19, 2016 at 12:25:55PM -0800, H. Peter Anvin wrote:
>> On 02/19/2016 05:45 AM, Luis R. Rodriguez wrote:
>> > +
>> > +/**
>> > + * DOC: Regular linker linker table constructors
>> > + *
>> > + * Regular constructors are expected to be used for valid linker table entries.
>> > + * Valid uses of weak entries other than the beginning and is currently
>> > + * untested but should in theory work.
>> > + */
>> > +
>> > +/**
>> > + * LINKTABLE_TEXT - Declares a linker table entry for execution
>> > + *
>> > + * @name: linker table name
>> > + * @level: order level
>> > + *
>> > + * Declares a linker table to be used for execution.
>> > + */
>> > +#define LINKTABLE_TEXT(name, level)                                        \
>> > +         __typeof__(name[0])                                       \
>> > +         __attribute__((used,                                      \
>> > +                        __aligned__(LINKTABLE_ALIGNMENT(name)),    \
>> > +                        section(SECTION_TBL(SECTION_TEXT, name, level))))
>>
>> I'm really confused by this.  Text should obviously be readonly,
>
> So this uses SECTION_TEXT, so we just pegged the linker table entry right below
> the standard SECTION_TEXT:

OK yes I see the issue now. I've modified this to use const, and
retested the kprobe patch and it works well still. kprobe would not
use LINKTABLE_TEXT, instead it uses its own macro, however users of
LINKTABLE_TEXT would then have const declared. The implications are
that you *can* declare structs so long as everything is const.

Folks may at times need to modify the structural definitions -- for
that LINKTABLE_DATA() is more appropriate, and as per my testing it
still allows execution of callbacks. I can document this.

Do we want .init.text to match this? I'd port my "struct x86_init_fn"
to use LINKTABLE_INIT_DATA() then. FWIW below is a paste of a simple
test program that demos what I mean.

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/workqueue.h>
#include <linux/tables.h>

static void stuff_todo(struct work_struct *work);
static DECLARE_WORK(stuff_work, stuff_todo);

struct stuff {
int a;
int b;
void (*print_a)(struct stuff *);
void (*print_b)(struct stuff *);

void (*print_a_ro)(const struct stuff *);
void (*print_b_ro)(const struct stuff *);
};

void print_a(struct stuff *s)
{
pr_info("print_a a: %d\n", s->a);
}

void print_a_ro(const struct stuff *s)
{
pr_info("print_a a: %d\n", s->a);
}

void print_b(struct stuff *s)
{
pr_info("print_b b: %d\n", s->b);
}

void print_b_ro(const struct stuff *s)
{
pr_info("print_b b: %d\n", s->b);
}

DEFINE_LINKTABLE_TEXT(struct stuff, my_stuff_fns_ro);
DEFINE_LINKTABLE_DATA(struct stuff, my_stuff_fns);

static LINKTABLE_TEXT(my_stuff_fns_ro, 0000) stuff_a_ro = {
.a = 1,
.b = 1,
.print_a_ro = print_a_ro,
.print_b_ro = print_b_ro,
};

static LINKTABLE_TEXT(my_stuff_fns_ro, 0000) stuff_b_ro = {
.a = 2,
.b = 2,
.print_a_ro = print_a_ro,
.print_b_ro = print_b_ro,
};

static LINKTABLE_TEXT(my_stuff_fns_ro, 0000) stuff_c_ro = {
.a = 3,
.b = 3,
.print_a_ro = print_a_ro,
.print_b_ro = print_b_ro,
};

static LINKTABLE_DATA(my_stuff_fns, 0000) stuff_a = {
.a = 1,
.b = 1,
.print_a = print_a,
.print_b = print_b,
};

static LINKTABLE_DATA(my_stuff_fns, 0000) stuff_b = {
.a = 2,
.b = 2,
.print_a = print_a,
.print_b = print_b,
};

static void stuff_todo(struct work_struct *work)
{
struct stuff *s;
const struct stuff *s_ro;
unsigned int i = 0;

pr_info("Looping over my_stuff_fns_ro\n");

LINKTABLE_FOR_EACH(s_ro, my_stuff_fns_ro) {
pr_info("Looping on s ro %d\n", i++);
s_ro->print_a_ro(s_ro);
s_ro->print_b_ro(s_ro);
}

i=0;
pr_info("Looping over my_stuff_fns\n");

LINKTABLE_FOR_EACH(s, my_stuff_fns) {
pr_info("Looping on s %d\n", i++);
s->print_a(s);
s->print_b(s);
}

i=0;
pr_info("Looping over my_stuff_fns and creating modifications\n");

LINKTABLE_FOR_EACH(s, my_stuff_fns) {
s->a = 10;
s->b = 10;
s->print_a(s);
s->print_b(s);
}
}

static int __init stuff_init(void)
{
/* get out of __init context */
schedule_work(&stuff_work);
return 0;
}

static void __exit stuff_exit(void)
{
cancel_work_sync(&stuff_work);
}

module_init(stuff_init)
module_exit(stuff_exit)
MODULE_LICENSE("GPL");


  Luis

[toc] | [next] | [standalone]

#1341166

On 02/23/2016 03:08 PM, Luis R. Rodriguez wrote:
> 
> OK yes I see the issue now. I've modified this to use const, and
> retested the kprobe patch and it works well still. kprobe would not
> use LINKTABLE_TEXT, instead it uses its own macro, however users of
> LINKTABLE_TEXT would then have const declared. The implications are
> that you *can* declare structs so long as everything is const.
> 

No, such structures belong in .rodata not in .text.

I have been thinking about it somewhat, and for text we really have text
"ranges" rather than tables.  The big difference between the two are:

1. priority ordering doesn't make any sense for ranges.
2. ranges can be hierarchial, that is, range "bar" can be entirely
   inside range "foo".
3. ranges aren't typed (although in C, that pretty much means they are
   "char" or "unsigned char" as there really isn't any way to define an
   "array of void".)
4. the only useful operator on a range is "is address X inside this
   range"; this operator is likely *not* useful for a table, since
   if you have to ever invoke it you are probably doing something very
   wrong.

For this to work, we need strings such that they will always sort in the
appropriate order with the bracket symbols around subranges.  I need to
think about this a few minutes before I actually say anything about it...

	-hpa

[toc] | [prev] | [next] | [standalone]

#1341177

On Tue, Feb 23, 2016 at 3:22 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 02/23/2016 03:08 PM, Luis R. Rodriguez wrote:
>>
>> OK yes I see the issue now. I've modified this to use const, and
>> retested the kprobe patch and it works well still. kprobe would not
>> use LINKTABLE_TEXT, instead it uses its own macro, however users of
>> LINKTABLE_TEXT would then have const declared. The implications are
>> that you *can* declare structs so long as everything is const.
>>
>
> No, such structures belong in .rodata not in .text.
>
> I have been thinking about it somewhat, and for text we really have text
> "ranges" rather than tables.  The big difference between the two are:
>
> 1. priority ordering doesn't make any sense for ranges.

I considered the possibility perhaps we want to just split out a
series of initial basic macros that enables *only* such basic
functionality and declarations, and linker tables would then build on
top of these. I think that ma help here.

> 2. ranges can be hierarchial, that is, range "bar" can be entirely
>    inside range "foo".

We'd want to enable this without having to modify the linker script
further -- I thought about how such things might be possible but not
for this case but rather, for the case where in linker tables a
developer may wish to get range addresses for a specific order level.
Although separate, the problem seems similar. At least in theory I was
convinced I had a solution to the order level problem, perhaps it may
suffice for this too:

When and if you need an intermediary set of range addresses you then
also declare another set of extern pointers, similar in nature with
the empty string and ~ ending delimiter, we just move one notch in.
SORT() will take care of ordering things for us, but the issue here is
the empty string doesn't allow hierarchies so perhaps to enable that
we may need another beginning delimiter that would enable recursive
hierarchies. If this seems to at least in theory make sense I can give
it a shot through a simple proof of concept and see if it works.

> 3. ranges aren't typed (although in C, that pretty much means they are
>    "char" or "unsigned char" as there really isn't any way to define an
>    "array of void".)

Sure. kprobe is a good example.

> 4. the only useful operator on a range is "is address X inside this
>    range"; this operator is likely *not* useful for a table, since
>    if you have to ever invoke it you are probably doing something very
>    wrong.

kprobe uses it :P

> For this to work, we need strings such that they will always sort in the
> appropriate order with the bracket symbols around subranges.  I need to
> think about this a few minutes before I actually say anything about it...

Ah seems we're in the same line of thought ?

 Luis

[toc] | [prev] | [next] | [standalone]

#1341200

On 02/23/2016 03:36 PM, Luis R. Rodriguez wrote:
> 
>> 4. the only useful operator on a range is "is address X inside this
>>    range"; this operator is likely *not* useful for a table, since
>>    if you have to ever invoke it you are probably doing something very
>>    wrong.
> 
> kprobe uses it :P
> 

Could you explain how?

	-hpa

[toc] | [prev] | [next] | [standalone]

#1341215

On Tue, Feb 23, 2016 at 04:06:55PM -0800, H. Peter Anvin wrote:
> On 02/23/2016 03:36 PM, Luis R. Rodriguez wrote:
> > 
> >> 4. the only useful operator on a range is "is address X inside this
> >>    range"; this operator is likely *not* useful for a table, since
                                                     ^^^^^^^^^^^^
> >>    if you have to ever invoke it you are probably doing something very
> >>    wrong.
> > 
> > kprobe uses it :P
> > 
> 
> Could you explain how?

Sorry I misread this as "unless you are a table", kprobes has two
ranges, one is a table (blacklist) and the other just a range
(for kprobes); only kprobes uses "address inside this range",
as reflected below. So I agree with you.

index d10ab6b9b5e0..d816c659f358 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1328,8 +1328,7 @@ out:
 bool __weak arch_within_kprobe_blacklist(unsigned long addr)
 {
        /* The __kprobes marked functions and entry code must not be probed */
-       return addr >= (unsigned long)__kprobes_text_start &&
-              addr < (unsigned long)__kprobes_text_end;
+       return LINKTABLE_ADDR_WITHIN(kprobes, addr);
 }
 
 bool within_kprobe_blacklist(unsigned long addr)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index d10ab6b9b5e0..d816c659f358 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1328,8 +1328,7 @@ out:
 bool __weak arch_within_kprobe_blacklist(unsigned long addr)
 {
        /* The __kprobes marked functions and entry code must not be probed */
-       return addr >= (unsigned long)__kprobes_text_start &&
-              addr < (unsigned long)__kprobes_text_end;
+       return LINKTABLE_ADDR_WITHIN(kprobes, addr);
 }
 
What about rebranding general section primitives under section.h

#define DECLARE_SECTION_TEXT_TYPE(type, name)                              \       
         extern const type name[], name##__end[];

#define DECLARE_SECTION_TEXT(name)                              \       
	DECLARE_SECTION_TEXT_TYPE(char, name)

Then tables.h would use the TYPE version:

#define DECLARE_LINKTABLE_TEXT(type, name)                              \       
         DECLARE_SECTION_TEXT_TYPE(type, name)

Since I've been making _TEXT the implicit type for section names(SECTION_INIT
is .init.text) the above could just be DECLARE_SECTION_TYPE() and DECLARE_SECTION()
for text if we prefer.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web