在 2020年08月12日 16:31, Mathias Krause 写道:
> Am 11.08.20 um 15:21 schrieb lijiang:
>>>> [...]
>>
>> I just know the details of the offset_table from Dave Anderson. The above changes will
>> break the extension modules that have been previously compiled, because the OFFSET()
>> values will be different.
>>
>> Would you mind correcting it and putting the new entries at end of the offset_table?
>
> Sure, will do. I wasn't aware of the conflict with already compiled
> extensions. Thanks for making me aware of that!
>
> Maybe we can try to "version" the offset structures so existing
> extensions won't simply use wrong offsets but error out.
>
It's still worth doing more investigations about this.
> Something like this, maybe? (untested, not even compiled):
>
> | diff --git a/defs.h b/defs.h
> | index d7adb23b86d5..8a4f68a7a5ee 100644
> | --- a/defs.h
> | +++ b/defs.h
> | @@ -2312,7 +2312,7 @@ struct array_table {
> | * The following set of macros can only be used with pre-intialized
> fields
> | * in the offset table, size table or array_table.
> | */
> | -#define OFFSET(X) (OFFSET_verify(offset_table.X, (char
> *)__FUNCTION__, __FILE__, __LINE__, #X))
> | +#define OFFSET(X) (OFFSET_verify(offset_table.X, (char
> *)__FUNCTION__, __FILE__, __LINE__, #X, sizeof(struct offset_table)))
> | #define SIZE(X) (SIZE_verify(size_table.X, (char
> *)__FUNCTION__, __FILE__, __LINE__, #X))
> | #define INVALID_OFFSET (-1)
> | #define INVALID_MEMBER(X) (offset_table.X == INVALID_OFFSET)
> | @@ -5220,7 +5220,7 @@ void clear_text_value_cache(void);
> | void dump_numargs_cache(void);
> | int patch_kernel_symbol(struct gnu_request *);
> | struct syment *generic_machdep_value_to_symbol(ulong, ulong *);
> | -long OFFSET_verify(long, char *, char *, int, char *);
> | +long OFFSET_verify(long, char *, char *, int, char *, size_t);
> | long SIZE_verify(long, char *, char *, int, char *);
> | long OFFSET_option(long, long, char *, char *, int, char *, char *);
> | long SIZE_option(long, long, char *, char *, int, char *, char *);
> | diff --git a/symbols.c b/symbols.c
> | index 3b1f08af43ff..3d0d0032ab85 100644
> | --- a/symbols.c
> | +++ b/symbols.c
> | @@ -13069,18 +13069,26 @@ SIZE_option(long size1, long size2, char
> *func, char *file, int line,
> | * to turn it off instead?
> | */
> | long
> | -OFFSET_verify(long offset, char *func, char *file, int line, char *item)
> | +OFFSET_verify(long offset, char *func, char *file, int line, char *item,
> | + size_t table_size)
> | {
> | char errmsg[BUFSIZE];
> |
> | + if (table_size != sizeof(struct offset_table)) {
> | + sprintf(errmsg, "offset table size mismatch: used %zu,
> but is %zu",
> | + table_size, sizeof(struct offset_table));
> | + goto err;
> | + }
> | +
> | if (!(pc->flags & DATADEBUG))
> | return offset;
> |
> | if (offset < 0) {
> | void *retaddr[NUMBER_STACKFRAMES] = { 0 };
> | - SAVE_RETURN_ADDRESS(retaddr);
> | sprintf(errmsg, "invalid structure member offset: %s",
> | item);
> | +err:
> | + SAVE_RETURN_ADDRESS(retaddr);
> | datatype_error(retaddr, errmsg, func, file, line);
> | }
> | return offset;
>
> We would need to extend it to cover all the other tables too, like
> size_table, array_table, kernel_table, etc. We just need to find proper
> places to hook these checks.
>
> Another sensible place might be register_extension() as that gets called
> during extension initialization. We would need make it a macro so it'll
> catch sizes that are used for compiling the actual extension. We then
> need to verify they match with the sizes used by crash. Something like
> this maybe -- again, completely untested:
>
I have to say that there are a lot of changes, and need to handle it carefully. :-)
> | diff --git a/defs.h b/defs.h
> | index d7adb23b86d5..0f87d75a8e76 100644
> | --- a/defs.h
> | +++ b/defs.h
> | @@ -2283,6 +2283,13 @@ struct array_table {
> | int vm_numa_stat;
> | };
> |
> | +struct table_sizes {
> | + size_t offset_table;
> | + size_t size_table;
> | + size_t array_table;
> | + //...
> | +};
> | +
> | /*
> | * The following set of macros use gdb to determine structure, union,
> | * or member sizes/offsets. They should be used only during
> initialization
> | @@ -5544,7 +5551,17 @@ void check_stack_overflow(void);
> | /*
> | * extensions.c
> | */
> | -void register_extension(struct command_table_entry *);
> | +#define register_extension(cte) \
> | + do { \
> | + static const struct table_sizes ts = { \
> | + .offset_table = sizeof(struct offset_table), \
> | + .size_table = sizeof(struct size_table), \
> | + .array_table = sizeof(struct array_table), \
> | + }; \
> | + _register_extension(cte, &ts); \
> | + } while (0)
> | +
> | +void _register_extension(struct command_table_entry *, const struct
> table_sizes *);
> | void dump_extension_table(int);
> | void load_extension(char *);
> | void unload_extension(char *);
> | diff --git a/extensions.c b/extensions.c
> | index d23b1e3cc26f..e64c2541010a 100644
> | --- a/extensions.c
> | +++ b/extensions.c
> | @@ -545,6 +545,17 @@ unload_extension(char *lib)
> | error(INFO, "%s: not loaded\n", lib);
> | }
> |
> | +static void verify_table_sizes(const struct table_sizes *tab_sizes)
> | +{
> | + if (tab_sizes == NULL)
> | + return;
> | +
> | + if (tab_sizes->offset_table != sizeof(struct offset_table))
> | + // error out
> | +
> | + // more size tests...
> | +}
> | +
> | /*
> | * Register the command_table as long as there are no command namespace
> | * clashes with the currently-existing command set. Also delete any
> aliases
> | @@ -556,10 +567,13 @@ unload_extension(char *lib)
> | * REGISTERED bit in the "current" extension_table structure flags.
> | */
> | void
> | -register_extension(struct command_table_entry *command_table)
> | +_register_extension(struct command_table_entry *command_table,
> | + const struct table_sizes *tab_sizes)
> | {
> | struct command_table_entry *cp;
> |
> | + verify_table_sizes(tab_sizes);
> | +
> | pc->curext->flags |= NO_MINIMAL_COMMANDS;
> |
> | for (cp = command_table; cp->name; cp++) {
> | @@ -604,6 +618,14 @@ register_extension(struct command_table_entry
> *command_table)
> | pc->curext->flags |= REGISTERED; /* Mark of
> approval */
> | }
> |
> | +/* for backwards compatibility with existing extensions */
> | +#undef register_extension
> | +void
> | +register_extension(struct command_table_entry *command_table)
> | +{
> | + _register_extension(command_table, NULL);
> | +}
> | +
> | /*
> | * Hooks for sial.
> | */
>
> But, maybe, this is all just band aid around real versioning of the data
> structures. We can simply have a global define that is the version
> number of crash's current definition of the extensions ABI. If we change
> some data structures, we just increment that number, making all existing
> extensions fail -- for good! Recompiling extensions should be required
> if the ABI changes, so trying to make it /maybe/ backwards compatible is
> just calling for trouble, IMHO. We should really error out in this case
It's important to keep the backward compatibility, although that may be
inconvenient.
> and not rely on developers to remember and keep the ordering.
>
Indeed, I agree with you. However, before the changes, we would still follow
the points for attention in writing crash code, which can avoid introducing
potential risks.
> What do you think?
>
I like the idea of improving it. But this seems to be beyond the scope of the
current patch, would you mind creating a new thread for this issue?
>> In addition, can you help to add the new entries to the dump_offset_table()?
>
> Sure. Will send a v2 with the minimal changes (adding new members to the
> end and to dump_offset_table()) next week.
>
OK, thank you, Mathias.
Lianbo
>> Sorry for this.
>
> No worries, that's what reviews are made for!
>
>
> Thanks,
> Mathias
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
