On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
It seems that users don't use `console_font` directly, they use `console_font_op`. Then, in TTY:
Wow, this is a maze :-/
(drivers/tty/vt/vt.c) int con_font_op(struct vc_data *vc, struct console_font_op *op) { switch (op->op) { case KD_FONT_OP_SET: return con_font_set(vc, op); case KD_FONT_OP_GET: return con_font_get(vc, op); case KD_FONT_OP_SET_DEFAULT: return con_font_default(vc, op); case KD_FONT_OP_COPY: return con_font_copy(vc, op); } return -ENOSYS; }
So my gut feeling is that this is just a bit of overenthusiastic common code sharing, and all it results is confuse everyone. I think if we change the conf_font_get/set/default/copy functions to not take the *op struct (which is take pretty arbitrarily from one of the ioctl), but the parameters each needs directly, that would clean up the code a _lot_. Since most callers would then directly call the right operation, instead of this detour through console_font_op. struct console_font_op is an uapi struct, so really shouldn't be used for internal abstractions - we can't change uapi, hence this makes it impossible to refactor anything from the get-go.
I also think that trying to get rid of con_font_op callers as much as possible (everywhere where the op struct is constructed in the kernel and doesn't come from userspace essentially) should be doable as a stand-alone patch series.
I see, I'll do some code searching and try to clean them up.
These 4 functions allocate `console_font`. We can replace them with our `kernel_console_font`. So, ...
$ vgrep ".con_font_set"
An aside: git grep is awesome, and really fast.
Ah, yes, by default vgrep uses git-grep. I use vgrep when I need to see something colorful :)
$ vgrep ".con_font_get" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1295 .con_font_get = sisusbcon_font_get, 1 drivers/video/console/vgacon.c 1227 .con_font_get = vgacon_font_get, 2 drivers/video/fbdev/core/fbcon.c 3121 .con_font_get = fbcon_get_font, $ $ vgrep ".con_font_default" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1379 .con_font_default = sisusbdummycon_font_default, 1 drivers/video/console/dummycon.c 163 .con_font_default = dummycon_font_default,
The above two return 0 but do nothing, which means width/height are now bogus (or well the same as what userspace set). I don't think that works correctly ...
2 drivers/video/console/newport_con.c 694 .con_font_default = newport_font_default,This just seems to release the userspace font. This is already done in other places where it makes a lot more sense to clean up.
3 drivers/video/fbdev/core/fbcon.c 3122 .con_font_default = fbcon_set_def_font,This actually does something. tbh I would not be surprises if the fb_set utility is the only thing that uses this - with a bit of code search we could perhaps confirm this, and delete all the other implementations.
$ vgrep ".con_font_copy" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1380 .con_font_copy = sisusbdummycon_font_copy, 1 drivers/video/console/dummycon.c 164 .con_font_copy = dummycon_font_copy,
Above two do nothing, but return 0. Again this wont work I think.
2 drivers/video/fbdev/core/fbcon.c 3123 .con_font_copy = fbcon_copy_font,Smells again like something that's only used by fb_set, and we could probably delete the other dummy implementations. Also I'm not even really clear on what this does ...
Removing these dummy functions means that for a dummy console these ioctls would start failing, but then I don't think anyone boots up into a dummy console and expects font changes to work. So again I think we could split this cleanup as prep work.
Sure, for step two, I'll read, confirm and try to remove these dummy functions.
... are these all the callbacks we need to take care of? What about other console drivers that don't register these callbacks? ...
... for example, mdacon.c? What font does mdacon.c use? I know that /lib/fonts/ exports two functions, find_font() and get_default_font(), but I don't see them being used in mdacon.c.
I think all other consoles either don't have fonts at all, or only support built-in fonts.
Ah, I see. I'll search for find_font() and get_default_font() when dealing with built-in fonts, then. These files are using them, in addition to fbcon.c:
drivers/firmware/efi/earlycon.c: font = get_default_font(xres, yres, -1, -1); drivers/video/console/sticore.c: fbfont = get_default_font(1024,768, ~(u32)0, ~(u32)0);
drivers/media/pci/solo6x10/solo6x10-enc.c: const struct font_desc *vga = find_font("VGA8x16"); drivers/media/test-drivers/vimc/vimc-core.c: const struct font_desc *font = find_font("VGA8x16"); drivers/media/test-drivers/vivid/vivid-core.c: const struct font_desc *font = find_font("VGA8x16"); drivers/usb/misc/sisusbvga/sisusb.c: myfont = find_font("VGA8x16"); drivers/video/console/sticore.c: fbfont = find_font(fbfont_name);
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
I see, that will also make handling built-in fonts much easier!
Currently `struct vc_data` contains a `struct console_font vc_font`, and I think this is making gradual conversion very hard. As an example, in fbcon_do_set_font(), we update `vc->vc_font`. We lose all the extra information we want in `kernel_console_font`, as long as `struct vc_data` still uses `console_font`...
However, if we let `struct vc_data` use `kernel_console_font` instead, we'll have to handle a lot of things in one go:
$ vgrep --no-less --no-header ".vc_font" | wc -l 296 $ echo ":(" :(
Yes :-/
This is essentially why the entire vc/fbcon layer is such a mess. It's a chaos, it doesn't really have clear abstraction, and very often the uapi structures (see also conf_font_op) leak deeply into the implementation, which means changing anything is nearly impossible ...
I think for vc_date->vc_font we might need a multi-step approach:
- first add a new helper function which sets the font for a vc using
an uapi console_font struct (and probably hard-coded assumes cnt == 256.
But user fonts may have a charcount different to 256... But yes I'll try to figure out how.
- roll that out everwhere
- change the type of vc_font to what we want (which should only need a
change in the helper function, which will also set charcount hopefully correctly, using the hard-coded assumption
- have another functions which sets the vf_font using a
kernel_console_font for all the cases where it matters
- now you can start using it and assume the charcount is set correctly
It's a journey unfortunately.
But at least it now sounds manageable! :)
Thank you, I'll look into this (especially the user charcount issue mentioned above) after cleaning up the uAPi structs and dummy functions.
The good news is, I've tried cleaning up all the macros in fbcon.c in my playground, and things seem to work. For example, currently we have:
if (userfont) cnt = FNTCHARCNT(data); else cnt = 256;After introducing `kernel_console_font` (and adding `charcount` to `struct font_desc` etc.), this should look like:
#define to_font(_data) container_of(_data, struct kernel_console_font, data) [...] cnt = to_font(data)->charcount;
Hm I guess we can't unify font_desc and the kernel_console_font we're talking about into one? I think that was brough up already somewhere else in this thread ...
Sure, let us use `font_desc` from now on.
No more `if` and `else`, and the framebuffer layer will be able to support new bulit-in fonts that have more than 256 characters. This seems really nice, so I'd like to spend some time working on it.
However before I start working on real patches, do you have suggestions about which console driver I should start with, or how should I split up the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
I think from a "stop security bugs" trying to clean up fbcon is the important part. That's also the most complex (only one that supports the default and copy functions it seems, and also one of the few that supports get). The other ones I think we should just try to not break. vgacon should still be useable (but I think only on systems where you can boot into legacy bios, not into uefi, at least on x86). I have no idea where some of the other consoles are even used.
For first steps I'd start with demidlayering some of the internal users of uapi structs, like the console_font_op really shouldn't be used anywhere in any function, except in the ioctl handler that converts it into the right function call. You'll probably discover a few other places like this on the go.
Sure, I'll start from this, then cleaning up these dummy functions, then `vc_data`. Thank you for the insights!
Peilin Ye