Re: [announce 7/7] fbsplash - documentation

[Michal Januszewski]

On Tue, Mar 08, 2005 at 04:18:07AM +0100, Arnd Bergmann wrote:

> It should probably just use its own hotplug agent instead of calling
> the helper directly.

I've just had a look at it, and it seems possible. From what I have seen
in the firmware_class.c code, it would require:
 - registering a class somewhere in the initializaton code
 - every time a request from fbcon is generated:
   - register the class device
   - create a timer
   - call kobject_hotplug() to send the event to userspace
   - unregister the device

This requests sent to userspace are generated while switching consoles
and resetting the graphics mode. This whole create device - send the
event - remove device thing seems a little long to me. Would it be
acceptable?
 
> > +Splash protocol v1 specified an additional 'fbsplash mode' after the 
> > +framebuffer number. Splash protocol v1 is deprecated and should not be used.
> When you're submitting a patch for inclusion, the interface should really
> be stable. I'd completely drop all references to the old version and only
> support one interface here. Moreover, you should not do versioned interfaces

The old one is already not supported. I mentioned v1 in the docs only so
that it would be clear where do the '2' in the helper's command line 
arguments come from. 

> unless you expect incompatible changes in future releases. As long as you
> still do, that is a sign that the patch is not ready for inclusion.

I don't do now, but who knows when changes will be made to fbcon in the
future? There's a possibility that things will change enough to justify
a new splash protocol. 

On the other hand, if the hotplug way of calling userspace were to be
adopted in fbsplash, the whole splash interface with the versioning
scheme could be dropped, which, I guess, would be a good thing.

> > + When the userspace helper is called in an early phase of the boot process
> > + (right after the initialization of fbcon), no filesystems will be mounted.
> > + The helper program should mount sysfs and then create the appropriate 
> > + framebuffer, fbsplash and tty0 devices (if they don't already exist) to get 
> > + current display settings and to be able to communicate with the kernel side.
> > + It should probably also mount the procfs to be able to parse the kernel 
> > + command line parameters.
> Nothing about the init command seems really necessary. Why not just do 
> that stuff from an /sbin/init script?

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. 

I'm currently using a setup that is a combination of both the 'quiet'
option and the 'init' command. I'm using the splash helper to switch to
a selected tty and display a full-screen image, while keeping the tty 
in KD_TEXT. This looks pretty nice and lets me see the initial bitmap
for ca. 5 secs.
 
> > +FB_SPLASH_IO_ORIG_KERNEL instructs fbsplash not to try to acquire the console
> > +semaphore. Not surprisingly, FB_SPLASH_IO_ORIG_USER instructs it to acquire
> > +the console sem.
> 
> 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.

To make sure no one uses fbsplash for malicious purposes, the
/dev/fbsplash device is used, which is kept root-writable only.

> > +Info on used structures:
> > +
> > +Definition of struct vc_splash can be found in linux/console_splash.h. It's
> > +heavily commented. Note that the 'theme' field should point to a string
> Not that heavily documented, actually ;-). 

Anything that needs some clearing-up?
 
> > +no longer than FB_SPLASH_THEME_LEN. When FBIOSPLASH_GETCFG call is 
> > +performed, the theme field should point to a char buffer of length
> > +FB_SPLASH_THEME_LEN.
> Then don't make it pointer but instead a field of that length. Pointers
> in ioctl arguments only cause trouble in mixed 32/64 bit environments.

That could be arranged, but this would require a separate structure for
communicating with userspace and with in-kernel data storage (currently
both these tasks are handled with struct vc_splash), or changing
vc_splash in vc_data to a pointer to a structure. The latter option
would be better I think.
 
> > +Definition of struct fb_splash_iowrapper can be found in linux/fb.h.
> > +The fields in this struct have the following meaning:
> > +
> > +vc: 
> > +Virtual console number.
> This should not be needed. I notice you are creating your own miscdevice
> for passing ioctl commands. That seems a little backwards since there
> already is a character device for the frame buffer. Can't you simply
> add your ioctl commands there?

I can, but:
- there are the previously-mentioned security reasons -- the user might
  wish to make the fb device writable to some less trusted people, while
  the fbsplash device should be kept protected,
- fbsplash calls aren't really related to a particular framebuffer
  device, they are related to a tty. 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).

> > +origin: 
> > +Specifies if the ioctl is performed as a response to a kernel request. The
> > +splash helper should set this field to FB_SPLASH_IO_ORIG_KERNEL, userspace
> > +programs should set it to FB_SPLASH_IO_ORIG_USER. This field is necessary to
> > +avoid console semaphore deadlocks.
> As I mentioned, this means there is thinko in your locking scheme.

It's not my locking scheme, it's the scheme that is currently used in
the whole console subsystem. If someone can find a sane way to correct
the problem and avoid the whole FB_SPLASH_IO_ORIG* thing, I would be
very heppy to update fbsplash's code.
 
> > +data: 
> > +Pointer to a data structure appropriate for the performed ioctl. Type of
> > +the data struct is specified in the ioctls description.
> This is very wrong. Using ioctl() for anything is bad enough because it
> contains an indirection from a system call. Here, you add yet another
> indirection. Instead of this, you should at least have a single data
> structure for each ioctl number, without using pointers to other structures.

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.

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