在 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@redhat.com https://www.redhat.com/mailman/listinfo/crash-utility