Re: Accessing external code or data from the 'console' kernel module: Can it be done?

[Andrei Borzenkov]

В Mon, 29 Jun 2015 19:43:39 +0200
Luc Van Rompaey <address@hidden> пишет:

> On 26/06/15 08:08, Andrei Borzenkov wrote:
> 
> > В Thu, 25 Jun 2015 21:23:59 +0200
> > Luc Van Rompaey <address@hidden> пишет:
> >
> >>
> >> On 24/06/15 08:37, Andrei Borzenkov wrote:
> >>> On Tue, Jun 23, 2015 at 8:35 PM, Luc Van Rompaey
> >>> <address@hidden> wrote:
> >>>> Make console driver export pointer to a table. This will make it
> >>>> possible for console driver to work without your module but provides
> >>>> access to this table if needed.
> >>>>
> >>>> Of course console driver will need to cope with NULL pointer (or
> >>>> initialize table to something meaningful).
> >>>>
> >>>> Hmmm... I think this is the one thing that I have not yet tried.
> >>>> If I understand correctly, I would then have to create a an EXPORT_VAR
> >>>> pointer variable in the console module, and let my 'setkey' module store 
> >>>> the
> >>>> appropriate value into it.
> >>>> The function to remap an input key, according to the contents of the 
> >>>> table,
> >>>> would then be a part of the console driver.
> >>> It can also be a function pointer. So code in console driver would reduce 
> >>> to
> >>>
> >>> if (grub_setkey_xlat)
> >>>    grub_setkey_xlat(...)
> >> I'm afraid I cannot get this to work, even though I'm trying to mimic
> >> some other code that appears to use the same mechanism.
> >> Here's what I'm doing:
> >>
> >> In console.c (i.e., 'grub-core/term/i386/pc/console.c'), I define the
> >> grub_setkey_xlat function pointer:
> >>
> >>     int (*grub_setkey_xlat) (int key) = NULL;
> >>
> >> Later on in the code, I do the "if (grub_setkey_xlat) blahblahblah"
> >> stuff as you suggested.
> >>
> >> These are the only changes to the 'console.c' source code, exactly as
> >> what happens in 'grub-core/kern/device.c' with the 'grub_net_open' 
> >> variable.
> >>
> >> Then, in my 'setkey.h' header file, I declare the following:
> >>
> >>     extern int (*EXPORT_VAR (grub_setkey_xlat)) (int key);
> >>
> > does conosle include setkey.h?
> 
> Yes, the 'console.c' source file includes the 'setkey.h' header file.
> In any case, I took a closer look at the issue, and there is clearly quite a 
> bit more going on than we are aware of here.
> 
> First, 'grub-core/Makefile' creates the list of exported kernel symbols based 
> on the applicable header files.
> It lists the header files in a variable, appropriately named 
> 'KERNEL_HEADER_FILES'.

Well, better name would be KERNEL_HEADER_FILES_WITH_EXPORTED_SYMBOLS.

> The 'setkey.h' header file does not occur in the KERNEL_HEADER_FILES value.
> Hence, and EXPORT_VAR and EXPORT_FUNC declarations in the 'setkey.h' header 
> file, will not get picked up by the build process.
> Any such symbols will, therefore, be considered undefined.
> 
> To test these assumptions, I decided to manually edit the Makefile (yes, I 
> know, I shouldn't do that, but it was a quick-and-dirty way to verify if I 
> understood the issue correctly).
> Right after the 'configure' step, and before I ran 'make', I manually 
> appended the path to my 'setkey' file to the KERNEL_HEADER_FILES value.
> It still didn't work (although the 'setkey.h' file did get picked up when the 
> list of exported kernel symbols was being constructed).
> Apparently, the declaration gets ignored because of the 'extern' keyword that 
> precedes it.
> 

No, that's very unlikely. Makefile just looks at EXPORT_VAR
(<alpha-numeric>*) pattern. It ignores anything that comes before or
after.

> So, I created a separate 'console_setkey.h' header file, in which I removed 
> the 'extern' keyword, and I modified the KERNEL_HEADER_FILES value 
> accordingly.
> This time, the build process completed without errors.
> What's more, the 'setkey' command that I had implemented worked perfectly as 
> well.
> 
> Since I couldn't immediately find out how I could influence the automatic 
> generation of the KERNEL_HEADER_FILES value, I then decided to try and add my 
> EXPORT_VAR declaration directly into the 'console.h' header file.
> Even though, at first, that seemed to work (no build errors occurred), my 
> 'setkey' command turned out to be non-functional (i.e., it no longer affected 
> the keyboard layout).
> In fact, there were now two instances of the 'grub_setkey_xlat' variable 
> defined within the kernel, as the following command output illustrates:
> 
> $ grep 'grub_setkey_xlat' 'grub/grub-core/syminfo.lst'
> defined linux grub_setkey_xlat
> defined vga grub_setkey_xlat

Sure. As you removed extern from declaration, every declaration became
separate definition, so now you have as many grub_setkey_xlat variables
defined as there are places that include your header. Unfortunately you
miss "defined kernel" which is what you actually need.

> undefined setkey grub_setkey_xlat
> 
> I assume that the 'linux' instance was the one being used by the console, 
> while, apparently, by sheer coincidence, my 'setkey' module must have 
> imported the 'vga' one.
> 
> SO - WHERE AM I NOW?
> 
> I have learned that:
> (1) my 'grub_setkey_xlat' variable (which is to be exported by the console) 
> will have to be defined in a header file that gets included by (and ONLY by) 
> 'console.c';

Again - this is misinterpretation. Many headers with EXPORT_* are
included in many places. Actually EXPORT_VAR is complete noop unless it
is compiled with special define.

> (2) the 'KERNEL_HEADER_FILES' variable in 'grub-core/Makefile' lists the 
> header files from which the exported kernel symbols will be picked up;
> (3) the KERNEL_HEADER_FILES value is not automatically updated as header 
> files are added to any of the kernel modules.
> 
> Additionally, I looks to me like the KERNEL_HEADER_FILES value is constructed 
> in the 'grub-core/Makefile.am' file (which, if I understand correctly, is an 
> automake input file).
> 
> In any case, I believe I now know enough of the details to allow me to 
> implement the final working version of my 'setkey' module.
> 
> All in all, it was quite an instructive exercise to research this subject!
>