Re: Crash-utility Digest, Vol 179, Issue 9

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



在 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




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux