Skip to contentSkip to conversation

ioctl32_unregister_conversion & modules

DISCUSSION Arnd Bergmann 3 participants 6 messages May 9, 2003 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Arnd Bergmann

author
#1 lore raw
Quoted 6 lines · Pavel Machek

...what is the problem?

It seems that function pointers into modules do not need any special treatmeant [I *know* there was talk about this on l-k; but I can't find anything in Documentation/]:

Quoted 7 lines
                if (!capable(CAP_SYS_ADMIN))
                        return -EACCES;
                if (disk->fops->ioctl) {
                        ret = disk->fops->ioctl(inode, file, cmd, arg);
                        if (ret != -EINVAL)
                                return ret;
                }

This is protected against unload by the reference counting done in open()/release(). ->ioctl() can be called only for open devices, so you know the ioctl handler is not getting unloaded while it is running.

Quoted 3 lines

So... what's the problem with {un}register_ioctl32_conversion being called from module_init/module_exit? Drivers in the tree do it already...

The problem is that when the conversion handler is called, the reference counting is only done for the module listed as ->owner in the file operations. For example in the patch you submitted to add register_ioctl32_conversion() to drivers/serial/core.c I see nothing stopping you from unloading core.ko while the handler is running on a device owned by drivers/char/cyclades.c or any other serial driver. It does not even have to be run on a serial driver, a user might try to do ioctl(TIOCGSERIAL, ...) on a regular file...

        Arnd <><
#2 lore raw

Hi!

Quoted 23 lines
Quoted 6 lines

...what is the problem?

It seems that function pointers into modules do not need any special treatmeant [I *know* there was talk about this on l-k; but I can't find anything in Documentation/]:

Quoted 7 lines
                if (!capable(CAP_SYS_ADMIN))
                        return -EACCES;
                if (disk->fops->ioctl) {
                        ret = disk->fops->ioctl(inode, file, cmd, arg);
                        if (ret != -EINVAL)
                                return ret;
                }

This is protected against unload by the reference counting done in open()/release(). ->ioctl() can be called only for open devices, so you know the ioctl handler is not getting unloaded while it is running.

Quoted 3 lines

So... what's the problem with {un}register_ioctl32_conversion being called from module_init/module_exit? Drivers in the tree do it already...

Quoted 8 lines

The problem is that when the conversion handler is called, the reference counting is only done for the module listed as ->owner in the file operations. For example in the patch you submitted to add register_ioctl32_conversion() to drivers/serial/core.c I see nothing stopping you from unloading core.ko while the handler is running on a device owned by drivers/char/cyclades.c or any other serial driver. It does not even have to be run on a serial driver, a user might try to do ioctl(TIOCGSERIAL, ...) on a regular file...

Oh.... Yep, that's pretty clear.

So what you are saying is that all existing register_ioctl32_conversion-s that are in unloadable module are broken. Ouch.

Fixing that would require resgister_ioctl32_conversion() to have 3-rd parameter "this module" and some magic inside fs/compat_ioctl.c, right?

Pavel

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#3 lore raw
Quoted 3 lines · Pavel Machek

Fixing that would require resgister_ioctl32_conversion() to have 3-rd parameter "this module" and some magic inside fs/compat_ioctl.c, right?

That would do it. I would suggest seperating out the static translation handlers and the dynamically registered ones. Now that you're adding in module owner info, the translation tables are going to start bloating up like crazy.

I'm still upset about going from 32-bit --> 64-bit entries on sparc64 :-(

Signature · 1 line
David S. Miller <[email protected]>
#4 lore raw

Hi!

Quoted 11 lines
Quoted 3 lines

Fixing that would require resgister_ioctl32_conversion() to have 3-rd parameter "this module" and some magic inside fs/compat_ioctl.c, right?

That would do it. I would suggest seperating out the static translation handlers and the dynamically registered ones. Now that you're adding in module owner info, the translation tables are going to start bloating up like crazy.

I'm still upset about going from 32-bit --> 64-bit entries on sparc64 :-(

So... Do you think moving common handlers from arch/*/ioctl32.c into fs/compat_ioctl.c would do the trick?
Pavel

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#5 lore raw

From: Pavel Machek <[email protected]>
Date: Fri, 9 May 2003 19:11:55 +0200

So... Do you think moving common handlers from arch/*/ioctl32.c into fs/compat_ioctl.c would do the trick?

Yes.

author
#6 lore raw
In reply to Pavel Machek
Quoted 3 lines · Pavel Machek

Fixing that would require resgister_ioctl32_conversion() to have 3-rd parameter "this module" and some magic inside fs/compat_ioctl.c, right?

The code that is currently using register_ioctl32_conversion() does not have to be changed if we use

extern int
__register_ioctl_conversion(int, ioctl_trans_handler_t, struct module*);

static inline int
register_ioctl_conversion(int cmd, ioctl_trans_handler_t h)

{
	return __register_ioctl_conversion(cmd, h, THIS_MODULE);
}

/* maybe also: */
static inline int
register_compatible_ioctl(int cmd)

{
	return __register_ioctl_conversion(cmd, NULL, NULL);
}

register_compatible_ioctl() is not strictly needed, but it will avoid doing the unnecessary try_module_get() when there is no handler.

	Arnd <><