Re: [announce 7/7] fbsplash - documentation

[Michal Januszewski]

On Wed, Mar 09, 2005 at 01:17:11AM +0100, Arnd Bergmann wrote:

> You also shouldn't create a class device every time you want to call
> kobject_hotplug. Note that every character device already has a class
> device associated with it, so you might be able to just use that to
> call kobject_hotplug on it.
> 
> If there is no obvious way to do that, just leave the code as it is
> for calling the helper.

OK, thanks forthe explanation and advice. I'll look into it.

> > The reason for calling init in that place is the ability to use the
> > userspace helper to display a nice picture while the kernel is still
> > loading. Sure, you can just drop it and use the 'quiet' command line
> > option, but that will give you a black screen for a good few seconds. 
> > And people who want eye-candy like fbsplash generally don't like
> > that. 
>
> Ok, understood. I think you should make that an extra patch and completely
> remove that feature from the base patchset in order to make it less
> controversial.

That can be done. The funny thing is, a week ago or so, it was proposed
in a thread about bootsplash that the code for the initial call could be
merged first and serve as a base for merging fbsplash :)

> > > That sounds really dangerous. Can you guarantee that nothing
> > > unexpected happens when a malicious user calls the ioctls with
> > > FB_SPLASH_IO_ORIG_KERNEL from a  regular user process?
> > 
> > This is pretty nasty, right, but I just can't see a way around it.
> > The thing is, when the splash helper is called from the kernel, the
> > console semaphore is already held so we have to avoid trying to acquire 
> > it again. And we can't just release it prior to calling the helper, as
> > it would break things badly.
> 
> I don't understand. Does the kernel code need to wait for the helper
> to finish while holding the semaphore? AFAICS, the helper is
> completely asynchronous, so it can simply do its job when the kernel 
> has given up the semaphore.

> >                                     And we can't do ioctls on ttys when 
> >   answering a kernel request because to the console sem problems
> >   (opening a tty = a call to acquire_console_sem(), which we need to
> >   avoid).
> 
> Hmm. One of us has misunderstood the interaction between 
> call_usermodehelper() and acquire_console_sem(). If I'm the one
> who's wrong, please tell me where that semaphore is held.

It think this will be best explained with an example. Consider the
following situation - we have two ttys - tty1 and tty2. tty1 is using
theme 'foo' an it's the foreground console. tty2 is using theme 'bar'.
Fbsplash is enabled on both these ttys.

Now the user decides to switch to tty2. He/she presses Alt+F2. The
keypress is handled somewhere and an item is added to the console_work
workqueue. console_callback() gets called asynchronously.
acquire_console_sem() is the first thing done in that function.

Now we have the following call stack (all done with the console sem
held):

change_console()
complete_change_console()
switch_screen() == redraw_screen()
con_switch() == fbcon_switch()

From inside fbcon_switch(), we need to call the splash helper to get a
new picture for the theme 'bar' which is used on tty2. The splash helper
does an ioctl and we're back in the kernel.. with the console semaphore
already held.

> > The original idea behind this was to group the fields that are
> > common to all fbsplash ioctl calls (ie. vc and origin) in one place. 
> > Of course, it can be changed to three differents structs, each 
> > containing the vc and origin fields and an int/struct vc_splash/struct 
> > fb_image, if that is the preferred way of doing things.
>
> It definitely is. Actually, it would be preferred to have only a
> single value as ioctl argument (or even better, no ioctl at all), 
> but if you need to pass an aggregate data type, it should at least 
> have an identical layout for all architectures. That means every 
> member should be naturally aligned and you can't use pointers or other
> types that have sizeof(x) == sizeof(long).

What are the alternatives to the ioctl? A relatively clean way of
feeding the data back to the kernel would be using sysfs. However, to
make it sane we would have to export the various components of struct
vc_splash in /sys/class/tty/tty<x> (otherwise, we would probabky have
to pass aggreaget data types through a sysfs entry -- something that is
IMO much worse than an ioctl). That however could be a little
problematic -- tty_class is currently defined as a class_simple.
To add entries other than the standard 'dev' we would have to define a
completely new class, right? That sounds like a rather intrusive
change..

Live long and prosper.
-- 
Michal 'Spock' Januszewski                        Gentoo Linux Developer
cell: +48504917690                         http://dev.gentoo.org/~spock/
JID: spock@xxxxxxxxxxxxx               freenode: #gentoo-dev, #gentoo-pl

Attachment: pgp00000.pgp
Description: PGP signature