Hi all,
Here is the second try for the atm refcount problem. I've made made
several enhancement over the previos patch. Can you take a look at it if
I've missed anything? (This time it also includes the driver for the
firestream card. That's why the patch is so large. It's gziped and
uuencoded).
Patrick
Patrick van de Lageweg wrote:
>
> Hi all,
>
> Here is the second try for the atm refcount problem. I've made made
> several enhancement over the previos patch. Can you take a look at it if
> I've missed anything? (This time it also includes the driver for the
> firestream card. That's why the patch is so large. It's gziped and
> uuencoded).
Patrick, I looked at the modules stuff and you do not
appear to be actually _using_ it anywhere:
bix:/home/morton> grep owner patch
+ owner: THIS_MODULE,
+ owner: THIS_MODULE
+ owner: THIS_MODULE,
+ owner: THIS_MODULE,
+ owner: THIS_MODULE,
+ owner: THIS_MODULE,
+ owner: THIS_MODULE,
+ struct module *owner;
+ struct module *owner;
bix:/home/morton>
It looks like you'll need something like the following:
(warning: uncompiled ATM-ignoramus code)
Index: net/atm/common.c
===================================================================
RCS file: /opt/cvs/lk/net/atm/common.c,v
retrieving revision 1.3.2.1
diff -u -u -r1.3.2.1 common.c
--- net/atm/common.c 2000/07/08 06:26:43 1.3.2.1
+++ net/atm/common.c 2000/10/27 11:17:45
@@ -144,6 +144,8 @@
"rx_inuse == %d after closing\n",
atomic_read(&vcc->rx_inuse));
+ if (vcc->dev->ops->owner)
+ __MOD_DEC_USE_COUNT(vcc->dev->ops->owner);
bind_vcc(vcc,NULL);
}
if (free_sk) free_atm_vcc_sk(sk);
}
@@ -199,13 +201,22 @@
{
int error;
+ if (try_inc_mod_count(dev->ops->owner) == 0) {
+ return -ENODEV;
+ }
+
+ error = 0;
+
if ((vpi != ATM_VPI_UNSPEC && vpi != ATM_VPI_ANY &&
vpi >> dev->ci_range.vpi_bits) || (vci != ATM_VCI_UNSPEC &&
- vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits))
- return -EINVAL;
- if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE))
- return -EPERM;
- error = 0;
+ vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits)) {
+ error = -EINVAL;
+ goto out;
+ }
+ if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE)) {
+ error = -EPERM;
+ goto out;
+ }
bind_vcc(vcc,dev);
switch (vcc->qos.aal) {
case ATM_AAL0:
@@ -231,19 +242,26 @@
if (!error) error = adjust_tp(&vcc->qos.rxtp,vcc->qos.aal);
if (error) {
bind_vcc(vcc,NULL);
- return error;
+ goto out;
}
DPRINTK("VCC %d.%d, AAL %d\n",vpi,vci,vcc->qos.aal);
DPRINTK(" TX: %d, PCR %d..%d, SDU %d\n",vcc->qos.txtp.traffic_class,
vcc->qos.txtp.min_pcr,vcc->qos.txtp.max_pcr,vcc->qos.txtp.max_sdu);
DPRINTK(" RX: %d, PCR %d..%d, SDU %d\n",vcc->qos.rxtp.traffic_class,
vcc->qos.rxtp.min_pcr,vcc->qos.rxtp.max_pcr,vcc->qos.rxtp.max_sdu);
+
if (dev->ops->open) {
error = dev->ops->open(vcc,vpi,vci);
if (error) {
bind_vcc(vcc,NULL);
- return error;
+ goto out;
}
+ }
+
+out:
+ if (error) {
+ if (dev->ops->owner)
+ __MOD_DEC_USE_COUNT(dev->ops->owner);
}
return 0;
}
Something similar will be need to be wrapped around the usage of
`struct atm_tcp_ops()' as well. Let me know if you'd like me to
prototype a patch for that.
The other thing you need to watch out for is atmdev_ops.ioctl().
Can this be called when the device is not open?
In other words, can atmdev_ops.ioctl() be called prior to
atmdev_ops.open()? In more other words, can ioctl() be
called after close()?
If so then the above patch is not sufficient - it only increments
the module use count on the open() path.
If this is the case then you're fairly severely screwed. This is
because the atm_dev handling has the same design flaw as the
netdevice handling: the logical place to inc/dec the module
refcount is within atm_dev_[de]register(). But this doesn't
work because you can never _get_ to the deregister point
through sys_delete_module() to drop the refcount.
Like netdevices, ATM needs to be able to separate the act
of loading the module from the act of registering the driver.
netdevices manage to get away with it because of ANK's funky
dev_hold()/dev_put() refcounting. It looks like ATM devices
aren't that lucky.
One workaround would be to refuse to allow the device to be
accessed at all if it isn't open. This may be unacceptable.
Look, this modules stuff is really bad. Phillip Rumpf proposed
a radical alternative a while back which I felt was not given
sufficient consideration. The idea was to make sys_delete_module()
grab all the other CPUs and leave them spinning on a flag while
the unload was proceeding. This was very similar to
arch/i386/kernel/apm.c:apm_power_off().
As far as I can recall, the only restriction was that you are
not allowed to call module functions when the module refcount
is zero if those functions can call schedule().
prumpf, please dig out that patch.
Andrew Morton wrote:
>
> Patrick van de Lageweg wrote:
> >
> > Hi all,
> >
> > Here is the second try for the atm refcount problem. I've made made
> > several enhancement over the previos patch. Can you take a look at it if
> > I've missed anything? (This time it also includes the driver for the
> > firestream card. That's why the patch is so large. It's gziped and
> > uuencoded).
[snip]
> The other thing you need to watch out for is atmdev_ops.ioctl().
> Can this be called when the device is not open?
>
> In other words, can atmdev_ops.ioctl() be called prior to
> atmdev_ops.open()? In more other words, can ioctl() be
> called after close()?
>
> If so then the above patch is not sufficient - it only increments
> the module use count on the open() path.
>
> If this is the case then you're fairly severely screwed. This is
> because the atm_dev handling has the same design flaw as the
> netdevice handling: the logical place to inc/dec the module
> refcount is within atm_dev_[de]register(). But this doesn't
> work because you can never _get_ to the deregister point
> through sys_delete_module() to drop the refcount.
The fact of the matter is that we seriously abuse ioctl() to provide an
interface to configuring network devices. You are supposed to have a
valid file descriptor to do an ioctl, but in the network config cases,
that fd doesn't have anything to do with the network device (look at the
special cases in net/*/af_*.c). Thus our strategy of managing the
module refcount on open/close gets sidestepped.
Currently, the network devices have two states: registered, and open.
Only in the open case is the module referenced. What should be done is
add a third state in the middle, configured, which also holds a
reference. Then the userspace configuration tools can explicitly
unconfigure the device in order to release the reference and unload the
module.
In the mean time, we could wrap refcount inc/dec around the calls to
dev->set_config() and dev->do_ioctl() in dev_ifsioc(). This fixes the
potential oops but still perpetuates the abuse of ioctl though. I would
rather see a new syscall or a misc character device added to provide the
configuration interface.
--
Brian Gerst
On Fri, 27 Oct 2000, Andrew Morton wrote:
> Patrick van de Lageweg wrote:
> >
> > Hi all,
> >
> > Here is the second try for the atm refcount problem. I've made made
> > several enhancement over the previos patch. Can you take a look at it if
> > I've missed anything? (This time it also includes the driver for the
> > firestream card. That's why the patch is so large. It's gziped and
> > uuencoded).
>
> Patrick, I looked at the modules stuff and you do not
> appear to be actually _using_ it anywhere:
>
> bix:/home/morton> grep owner patch
> + owner: THIS_MODULE,
> + owner: THIS_MODULE
> + owner: THIS_MODULE,
> + owner: THIS_MODULE,
> + owner: THIS_MODULE,
> + owner: THIS_MODULE,
> + owner: THIS_MODULE,
> + struct module *owner;
> + struct module *owner;
> bix:/home/morton>
We use it throught the fops_get/fops_put macros to in/decrease the mod
counter. See the definitions for those macros (include/linux/fs.h)
Patrick
>
>
> It looks like you'll need something like the following:
> (warning: uncompiled ATM-ignoramus code)
>
> Index: net/atm/common.c
> ===================================================================
> RCS file: /opt/cvs/lk/net/atm/common.c,v
> retrieving revision 1.3.2.1
> diff -u -u -r1.3.2.1 common.c
> --- net/atm/common.c 2000/07/08 06:26:43 1.3.2.1
> +++ net/atm/common.c 2000/10/27 11:17:45
> @@ -144,6 +144,8 @@
> "rx_inuse == %d after closing\n",
> atomic_read(&vcc->rx_inuse));
> + if (vcc->dev->ops->owner)
> + __MOD_DEC_USE_COUNT(vcc->dev->ops->owner);
> bind_vcc(vcc,NULL);
> }
> if (free_sk) free_atm_vcc_sk(sk);
> }
> @@ -199,13 +201,22 @@
> {
> int error;
>
> + if (try_inc_mod_count(dev->ops->owner) == 0) {
> + return -ENODEV;
> + }
> +
> + error = 0;
> +
> if ((vpi != ATM_VPI_UNSPEC && vpi != ATM_VPI_ANY &&
> vpi >> dev->ci_range.vpi_bits) || (vci != ATM_VCI_UNSPEC &&
> - vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits))
> - return -EINVAL;
> - if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE))
> - return -EPERM;
> - error = 0;
> + vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits)) {
> + error = -EINVAL;
> + goto out;
> + }
> + if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE)) {
> + error = -EPERM;
> + goto out;
> + }
> bind_vcc(vcc,dev);
> switch (vcc->qos.aal) {
> case ATM_AAL0:
> @@ -231,19 +242,26 @@
> if (!error) error = adjust_tp(&vcc->qos.rxtp,vcc->qos.aal);
> if (error) {
> bind_vcc(vcc,NULL);
> - return error;
> + goto out;
> }
> DPRINTK("VCC %d.%d, AAL %d\n",vpi,vci,vcc->qos.aal);
> DPRINTK(" TX: %d, PCR %d..%d, SDU %d\n",vcc->qos.txtp.traffic_class,
> vcc->qos.txtp.min_pcr,vcc->qos.txtp.max_pcr,vcc->qos.txtp.max_sdu);
> DPRINTK(" RX: %d, PCR %d..%d, SDU %d\n",vcc->qos.rxtp.traffic_class,
> vcc->qos.rxtp.min_pcr,vcc->qos.rxtp.max_pcr,vcc->qos.rxtp.max_sdu);
> +
> if (dev->ops->open) {
> error = dev->ops->open(vcc,vpi,vci);
> if (error) {
> bind_vcc(vcc,NULL);
> - return error;
> + goto out;
> }
> + }
> +
> +out:
> + if (error) {
> + if (dev->ops->owner)
> + __MOD_DEC_USE_COUNT(dev->ops->owner);
> }
> return 0;
> }
>
>
> Something similar will be need to be wrapped around the usage of
> `struct atm_tcp_ops()' as well. Let me know if you'd like me to
> prototype a patch for that.
>
> The other thing you need to watch out for is atmdev_ops.ioctl().
> Can this be called when the device is not open?
>
> In other words, can atmdev_ops.ioctl() be called prior to
> atmdev_ops.open()? In more other words, can ioctl() be
> called after close()?
>
> If so then the above patch is not sufficient - it only increments
> the module use count on the open() path.
>
> If this is the case then you're fairly severely screwed. This is
> because the atm_dev handling has the same design flaw as the
> netdevice handling: the logical place to inc/dec the module
> refcount is within atm_dev_[de]register(). But this doesn't
> work because you can never _get_ to the deregister point
> through sys_delete_module() to drop the refcount.
>
> Like netdevices, ATM needs to be able to separate the act
> of loading the module from the act of registering the driver.
>
> netdevices manage to get away with it because of ANK's funky
> dev_hold()/dev_put() refcounting. It looks like ATM devices
> aren't that lucky.
>
> One workaround would be to refuse to allow the device to be
> accessed at all if it isn't open. This may be unacceptable.
>
>
> Look, this modules stuff is really bad. Phillip Rumpf proposed
> a radical alternative a while back which I felt was not given
> sufficient consideration. The idea was to make sys_delete_module()
> grab all the other CPUs and leave them spinning on a flag while
> the unload was proceeding. This was very similar to
> arch/i386/kernel/apm.c:apm_power_off().
>
> As far as I can recall, the only restriction was that you are
> not allowed to call module functions when the module refcount
> is zero if those functions can call schedule().
>
> prumpf, please dig out that patch.
>
Patrick van de Lageweg wrote:
>
> On Fri, 27 Oct 2000, Andrew Morton wrote:
>
> > Patrick van de Lageweg wrote:
> > >
> > > Hi all,
> > >
> > > Here is the second try for the atm refcount problem. I've made made
> > > several enhancement over the previos patch. Can you take a look at it if
> > > I've missed anything? (This time it also includes the driver for the
> > > firestream card. That's why the patch is so large. It's gziped and
> > > uuencoded).
> >
> > Patrick, I looked at the modules stuff and you do not
> > appear to be actually _using_ it anywhere:
> >
> > bix:/home/morton> grep owner patch
> > + owner: THIS_MODULE,
> > + owner: THIS_MODULE
> > + owner: THIS_MODULE,
> > + owner: THIS_MODULE,
> > + owner: THIS_MODULE,
> > + owner: THIS_MODULE,
> > + owner: THIS_MODULE,
> > + struct module *owner;
> > + struct module *owner;
> > bix:/home/morton>
>
> We use it throught the fops_get/fops_put macros to in/decrease the mod
> counter. See the definitions for those macros (include/linux/fs.h)
>
> Patrick
This will break horribly if fops_put/get are changed to inlines instead
of macros. They are only supposed to be used on struct file_operations.
--
Brian Gerst
Brian Gerst wrote:
> > > + struct module *owner;
> > > + struct module *owner;
> > > bix:/home/morton>
> >
> > We use it throught the fops_get/fops_put macros to in/decrease the mod
> > counter. See the definitions for those macros (include/linux/fs.h)
> >
> > Patrick
>
> This will break horribly if fops_put/get are changed to inlines instead
> of macros. They are only supposed to be used on struct file_operations.
Oh?
Anyway, we'll get nice warnings about wrong type of argument when that
happens.
I was the one who found the fops_get/put code useful as a guideline
and also in fact as the code to call.
So the question is: What is the defined interface for fops_get/put: Is
it "it's a macro that... " or is it "it's a function (possibly a macro
for efficiency) that.... "?
Roger.
P.S. Apologies for Patrick's bad quoting habits: He had to catch a
train and forgot to delete the rest of the quoted mail.
--
** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2137555 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
* Common sense is the collection of *
****** prejudices acquired by age eighteen. -- Albert Einstein ********
On Fri, Oct 27, 2000 at 10:49:53PM +1100, Andrew Morton wrote:
> Look, this modules stuff is really bad. Phillip Rumpf proposed
> a radical alternative a while back which I felt was not given
While it might be a "radical alternative", it doesn't require any changes
to the subsystems that have been fixed so far. At this time, applying the
patch would basically fix the rest of the subsystems as well (if the
drivers use MOD_{INC,DEC}_USE_COUNT, that is).
> sufficient consideration. The idea was to make sys_delete_module()
> grab all the other CPUs and leave them spinning on a flag while
> the unload was proceeding. This was very similar to
> arch/i386/kernel/apm.c:apm_power_off().
The idea here is other CPUs don't have to deal with the kernel going
through a number of inconsistent states while a module is unloaded. At
any point in time, for any module, exactly one of the following is true:
1. you're in the module_exit function
2. the module is (being) loaded
3. the module isn't loaded.
> As far as I can recall, the only restriction was that you are
> not allowed to call module functions when the module refcount
> is zero if those functions can call schedule().
There are other restrictions which shouldn't really matter:
- you can't schedule() and hope you end up on a particular CPU (you can
use smp_call_function though)
- you can't copy_(from|to)_user in the module exit function (which would
be copies from/to rmmod anyway)
> prumpf, please dig out that patch.
attached (rediff against test10-pre6, it seems to work).
Philipp Rumpf wrote:
>
> On Fri, Oct 27, 2000 at 10:49:53PM +1100, Andrew Morton wrote:
> > Look, this modules stuff is really bad. Phillip Rumpf proposed
> > a radical alternative a while back which I felt was not given
>
> While it might be a "radical alternative", it doesn't require any changes
> to the subsystems that have been fixed so far. At this time, applying the
> patch would basically fix the rest of the subsystems as well (if the
> drivers use MOD_{INC,DEC}_USE_COUNT, that is).
>
> > sufficient consideration. The idea was to make sys_delete_module()
> > grab all the other CPUs and leave them spinning on a flag while
> > the unload was proceeding. This was very similar to
> > arch/i386/kernel/apm.c:apm_power_off().
>
> The idea here is other CPUs don't have to deal with the kernel going
> through a number of inconsistent states while a module is unloaded. At
> any point in time, for any module, exactly one of the following is true:
>
> 1. you're in the module_exit function
> 2. the module is (being) loaded
> 3. the module isn't loaded.
>
> > As far as I can recall, the only restriction was that you are
> > not allowed to call module functions when the module refcount
> > is zero if those functions can call schedule().
>
> There are other restrictions which shouldn't really matter:
>
> - you can't schedule() and hope you end up on a particular CPU (you can
> use smp_call_function though)
>
> - you can't copy_(from|to)_user in the module exit function (which would
> be copies from/to rmmod anyway)
Unfortunately, you need to be able to use copy_*_user() from the network
ioctls, and this is the center of the current issue.
--
Brian Gerst
On Sat, Oct 28, 2000 at 09:37:28AM -0400, Brian Gerst wrote:
> Philipp Rumpf wrote:
> > - you can't copy_(from|to)_user in the module exit function (which would
> > be copies from/to rmmod anyway)
>
> Unfortunately, you need to be able to use copy_*_user() from the network
> ioctls, and this is the center of the current issue.
You misunderstood. The network ioctls aren't module_exit functions, so
copy_*_user in them is fine.
Philipp Rumpf
Philipp Rumpf wrote:
>
> On Sat, Oct 28, 2000 at 09:37:28AM -0400, Brian Gerst wrote:
> > Philipp Rumpf wrote:
> > > - you can't copy_(from|to)_user in the module exit function (which would
> > > be copies from/to rmmod anyway)
> >
> > Unfortunately, you need to be able to use copy_*_user() from the network
> > ioctls, and this is the center of the current issue.
>
> You misunderstood. The network ioctls aren't module_exit functions, so
> copy_*_user in them is fine.
Yes, but they can be called (and sleep) with module refcount == 0. This
is because the file descripter used to perform the ioctl isn't directly
associated with the network device, thereby not incrementing the
refcount on open.
--
Brian Gerst
On Sat, Oct 28, 2000 at 09:55:21AM -0400, Brian Gerst wrote:
> Yes, but they can be called (and sleep) with module refcount == 0. This
> is because the file descripter used to perform the ioctl isn't directly
> associated with the network device, thereby not incrementing the
> refcount on open.
According to my proposal, it is perfectly safe to call a function in a module
while the module's use count is 0. This function would typically look like this:
foo()
{
MOD_INC_USE_COUNT;
copy_*_user() (or anything else that sleeps);
MOD_DEC_USE_COUNT;
return bar;
}
The only difference to the "old" module scheme is that the above currently isn't
safe on SMP systems.
Philipp Rumpf wrote:
>
> On Sat, Oct 28, 2000 at 09:55:21AM -0400, Brian Gerst wrote:
> > Yes, but they can be called (and sleep) with module refcount == 0. This
> > is because the file descripter used to perform the ioctl isn't directly
> > associated with the network device, thereby not incrementing the
> > refcount on open.
>
> According to my proposal, it is perfectly safe to call a function in a module
> while the module's use count is 0. This function would typically look like this:
>
> foo()
> {
> MOD_INC_USE_COUNT;
>
> copy_*_user() (or anything else that sleeps);
>
> MOD_DEC_USE_COUNT;
>
> return bar;
> }
>
> The only difference to the "old" module scheme is that the above currently isn't
> safe on SMP systems.
This will only work while the kernel is not preemptable. Once the
kernel thread can be rescheduled, all bets are off.
With or without your patch, the network ioctls are unsafe, since they
don't currently do refcounting at all. Adding it in the layer above the
driver is the easier and cleaner solution.
--
Brian Gerst
Brian Gerst wrote:
>
> With or without your patch, the network ioctls are unsafe, since they
> don't currently do refcounting at all. Adding it in the layer above the
> driver is the easier and cleaner solution.
As long as the drivers use unregister_netdevice() then that's
fairly easy to fix within the netdevice layer. Just do a
dev_hold()/dev_put() within dev_ifsioc().
On Sat, Oct 28, 2000 at 11:50:22AM -0400, Brian Gerst wrote:
> Philipp Rumpf wrote:
> >
> > On Sat, Oct 28, 2000 at 09:55:21AM -0400, Brian Gerst wrote:
> > > Yes, but they can be called (and sleep) with module refcount == 0. This
> > > is because the file descripter used to perform the ioctl isn't directly
> > > associated with the network device, thereby not incrementing the
> > > refcount on open.
> >
> > According to my proposal, it is perfectly safe to call a function in a module
> > while the module's use count is 0. This function would typically look like this:
> >
> > foo()
> > {
> > MOD_INC_USE_COUNT;
> >
> > copy_*_user() (or anything else that sleeps);
> >
> > MOD_DEC_USE_COUNT;
> >
> > return bar;
> > }
> >
> > The only difference to the "old" module scheme is that the above currently isn't
> > safe on SMP systems.
>
> This will only work while the kernel is not preemptable. Once the
> kernel thread can be rescheduled, all bets are off.
The implementation will need adjusting for preemptable kernel threads. The
concept still works fine.
> With or without your patch, the network ioctls are unsafe, since they
> don't currently do refcounting at all.
I was under the impression most network ioctls didn't sleep.
> Adding it in the layer above thTe
> driver is the easier and cleaner solution.
I disagree. I dislike special-casing inter-module calls (and that's not even
taking into account that the current implementation of an inter-module call is
quite ugly).