Re: CVS commit: src/sys/opencrypto
On Sun, 19 Jan 2014, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Sun Jan 19 18:16:13 UTC 2014 Modified Files: src/sys/opencrypto: cryptodev.c Log Message: bail out unloading for now How about the following changes? @@ -143,6 +143,8 @@ static int cryptoread(dev_t dev, struct static int cryptowrite(dev_t dev, struct uio *uio, int ioflag); static int cryptoselect(dev_t dev, int rw, struct lwp *l); +static int crypto_refcount = 0;/* Prevent detaching while in use */ + /* Declaration of cloned-device (per-ctxt) entrypoints */ static int cryptof_read(struct file *, off_t *, struct uio *, kauth_cred_t, int); @@ -262,6 +264,7 @@ cryptof_ioctl(struct file *fp, u_long cm */ criofcr->sesn = 1; criofcr->requestid = 1; + crypto_refcount++; mutex_exit(&crypto_mtx); (void)fd_clone(criofp, criofd, (FREAD|FWRITE), &cryptofops, criofcr); @@ -951,6 +954,7 @@ cryptof_close(struct file *fp) } seldestroy(&fcr->sinfo); fp->f_data = NULL; + crypto_refcount--; mutex_exit(&crypto_mtx); pool_put(&fcrpl, fcr); @@ -1080,6 +1084,7 @@ cryptoopen(dev_t dev, int flag, int mode */ fcr->sesn = 1; fcr->requestid = 1; + crypto_refcount++; mutex_exit(&crypto_mtx); return fd_clone(fp, fd, flag, &cryptofops, fcr); } @@ -2109,6 +2114,10 @@ int crypto_detach(device_t, int); int crypto_detach(device_t self, int num) { + + if (crypto_refcount != 0 || self->dv_unit != 0) + return EBUSY; + pool_destroy(&fcrpl); pool_destroy(&csepl); To generate a diff of this commit: cvs rdiff -u -r1.71 -r1.72 src/sys/opencrypto/cryptodev.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. !DSPAM:52dc1675236281199521295! - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
On Jan 19, 10:22am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | How about the following changes? You need to handle the regular open too, not justthe get (look for the other fd_clone) | @@ -143,6 +143,8 @@ static intcryptoread(dev_t dev, struct | static int cryptowrite(dev_t dev, struct uio *uio, int ioflag); | static int cryptoselect(dev_t dev, int rw, struct lwp *l); | | +static int crypto_refcount = 0;/* Prevent detaching while in use */ | + | /* Declaration of cloned-device (per-ctxt) entrypoints */ | static int cryptof_read(struct file *, off_t *, struct uio *, | kauth_cred_t, int); | @@ -262,6 +264,7 @@ cryptof_ioctl(struct file *fp, u_long cm |*/ | criofcr->sesn = 1; | criofcr->requestid = 1; | + crypto_refcount++; | mutex_exit(&crypto_mtx); | (void)fd_clone(criofp, criofd, (FREAD|FWRITE), | &cryptofops, criofcr); | @@ -951,6 +954,7 @@ cryptof_close(struct file *fp) | } | seldestroy(&fcr->sinfo); | fp->f_data = NULL; | + crypto_refcount--; | mutex_exit(&crypto_mtx); | | pool_put(&fcrpl, fcr); | @@ -1080,6 +1084,7 @@ cryptoopen(dev_t dev, int flag, int mode |*/ | fcr->sesn = 1; | fcr->requestid = 1; | + crypto_refcount++; | mutex_exit(&crypto_mtx); | return fd_clone(fp, fd, flag, &cryptofops, fcr); | } | @@ -2109,6 +2114,10 @@ intcrypto_detach(device_t, int); It is not just the detach we need to handle, it is the module unload too (look for FINI). | int | crypto_detach(device_t self, int num) | { | + | + if (crypto_refcount != 0 || self->dv_unit != 0) | + return EBUSY; | + | pool_destroy(&fcrpl); | pool_destroy(&csepl); christos
Re: CVS commit: src/sys/opencrypto
On Sun, 19 Jan 2014, Christos Zoulas wrote: On Jan 19, 10:22am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | How about the following changes? You need to handle the regular open too, not justthe get (look for the other fd_clone) That's covered in cryptoopen() at line 1060 | @@ -143,6 +143,8 @@ static intcryptoread(dev_t dev, struct | static int cryptowrite(dev_t dev, struct uio *uio, int ioflag); | static int cryptoselect(dev_t dev, int rw, struct lwp *l); | | +static int crypto_refcount = 0;/* Prevent detaching while in use */ | + | /* Declaration of cloned-device (per-ctxt) entrypoints */ | static int cryptof_read(struct file *, off_t *, struct uio *, | kauth_cred_t, int); | @@ -262,6 +264,7 @@ cryptof_ioctl(struct file *fp, u_long cm |*/ | criofcr->sesn = 1; | criofcr->requestid = 1; | + crypto_refcount++; | mutex_exit(&crypto_mtx); | (void)fd_clone(criofp, criofd, (FREAD|FWRITE), | &cryptofops, criofcr); | @@ -951,6 +954,7 @@ cryptof_close(struct file *fp) | } | seldestroy(&fcr->sinfo); | fp->f_data = NULL; | + crypto_refcount--; | mutex_exit(&crypto_mtx); | | pool_put(&fcrpl, fcr); | @@ -1080,6 +1084,7 @@ cryptoopen(dev_t dev, int flag, int mode |*/ | fcr->sesn = 1; | fcr->requestid = 1; | + crypto_refcount++; | mutex_exit(&crypto_mtx); | return fd_clone(fp, fd, flag, &cryptofops, fcr); | } | @@ -2109,6 +2114,10 @@ intcrypto_detach(device_t, int); It is not just the detach we need to handle, it is the module unload too (look for FINI). Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() which attempts to detach each device instance. If a detach fails, then config_cfdata_detach fails, and the unload will fail. | int | crypto_detach(device_t self, int num) | { | + | + if (crypto_refcount != 0 || self->dv_unit != 0) | + return EBUSY; | + | pool_destroy(&fcrpl); | pool_destroy(&csepl); - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
On Jan 19, 10:37am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | That's covered in cryptoopen() at line 1060 I missed that patch | Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() | which attempts to detach each device instance. If a detach fails, then | config_cfdata_detach fails, and the unload will fail. Ok then! Did you test it? If it works, I guess commit it. christos
Re: CVS commit: src/sys/opencrypto
On Sun, 19 Jan 2014, Christos Zoulas wrote: On Jan 19, 10:37am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | That's covered in cryptoopen() at line 1060 I missed that patch No worry. | Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() | which attempts to detach each device instance. If a detach fails, then | config_cfdata_detach fails, and the unload will fail. Ok then! Did you test it? If it works, I guess commit it. It does address the simple case I described. It has a minor/cosmetic issue of printing an error message crypto0: unable to detach instance from config_cfdata_detach(). But David Laight's post has me more concerned, that perhaps we really need to solve this sort of issue more generically. So, not sure if this should be committed, or if we should leave your code in to prevent unload in all cases. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
On Jan 19, 11:21am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | > | Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() | > | which attempts to detach each device instance. If a detach fails, then | > | config_cfdata_detach fails, and the unload will fail. | > | > Ok then! Did you test it? If it works, I guess commit it. | | It does address the simple case I described. It has a minor/cosmetic | issue of printing an error message | | crypto0: unable to detach instance | | from config_cfdata_detach(). yes, it looks like it is not designed to be called if it is not going to work. This is why most other things do the test in the MODULE_FINI part, see bpf for example. | But David Laight's post has me more concerned, that perhaps we really | need to solve this sort of issue more generically. Yes, this is why I took the EOPNOTSUPP shortcut. | So, not sure if this should be committed, or if we should leave your | code in to prevent unload in all cases. Well, you can move your test from detach to MODULE_FINI and it should work just fine. But yes, we should solve it more generically, but I still think the reference counting code is needed. christos
Re: CVS commit: src/sys/opencrypto
On Sun, 19 Jan 2014, John Nemeth wrote: } Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() } which attempts to detach each device instance. If a detach fails, then } config_cfdata_detach fails, and the unload will fail. Does this mean that you'll end up with some device instances detached and not others? Nope. All non-zero units are prevented from unload. And unit zero is permitted only if there are no references, ie no clones. So the master gets deleted only if there are no non-zero units. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
In article , Paul Goyette wrote: >On Sun, 19 Jan 2014, John Nemeth wrote: > >> } Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() >> } which attempts to detach each device instance. If a detach fails, then >> } config_cfdata_detach fails, and the unload will fail. >> >> Does this mean that you'll end up with some device instances >> detached and not others? > >Nope. > >All non-zero units are prevented from unload. And unit zero is >permitted only if there are no references, ie no clones. I don't see why non-zero units are special, can you explain? Open zero open 1, close 0, close 1. Should the close 1 unload? christos
Re: CVS commit: src/sys/opencrypto
On Jan 19, 10:37am, Paul Goyette wrote: } On Sun, 19 Jan 2014, Christos Zoulas wrote: } > On Jan 19, 10:22am, p...@whooppee.com (Paul Goyette) wrote: } > } > | How about the following changes? } > } > You need to handle the regular open too, not justthe get (look for the } > other fd_clone) } } That's covered in cryptoopen() at line 1060 } } > | @@ -143,6 +143,8 @@ static intcryptoread(dev_t dev, struct } > | static int cryptowrite(dev_t dev, struct uio *uio, int ioflag); } > | static int cryptoselect(dev_t dev, int rw, struct lwp *l); } > | } > | +static int crypto_refcount = 0;/* Prevent detaching while in use */ } > | + } > | /* Declaration of cloned-device (per-ctxt) entrypoints */ } > | static int cryptof_read(struct file *, off_t *, struct uio *, } > | kauth_cred_t, int); } > | @@ -262,6 +264,7 @@ cryptof_ioctl(struct file *fp, u_long cm } > |*/ } > | criofcr->sesn = 1; } > | criofcr->requestid = 1; } > | + crypto_refcount++; } > | mutex_exit(&crypto_mtx); } > | (void)fd_clone(criofp, criofd, (FREAD|FWRITE), } > | &cryptofops, criofcr); } > | @@ -951,6 +954,7 @@ cryptof_close(struct file *fp) } > | } } > | seldestroy(&fcr->sinfo); } > | fp->f_data = NULL; } > | + crypto_refcount--; } > | mutex_exit(&crypto_mtx); } > | } > | pool_put(&fcrpl, fcr); } > | @@ -1080,6 +1084,7 @@ cryptoopen(dev_t dev, int flag, int mode } > |*/ } > | fcr->sesn = 1; } > | fcr->requestid = 1; } > | + crypto_refcount++; } > | mutex_exit(&crypto_mtx); } > | return fd_clone(fp, fd, flag, &cryptofops, fcr); } > | } } > | @@ -2109,6 +2114,10 @@ intcrypto_detach(device_t, int); } > } > It is not just the detach we need to handle, it is the module unload too } > (look for FINI). } } Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() } which attempts to detach each device instance. If a detach fails, then } config_cfdata_detach fails, and the unload will fail. Does this mean that you'll end up with some device instances detached and not others? }-- End of excerpt from Paul Goyette
Re: CVS commit: src/sys/opencrypto
The mere existence of a non-zero unit is a "reference" that needs to prevent unloading. The two checks (unit# and ref-count) are equivalent and redundant, and only one of them needs to be there. On Sun, 19 Jan 2014, Christos Zoulas wrote: In article , Paul Goyette wrote: On Sun, 19 Jan 2014, John Nemeth wrote: } Handled indirectly. The MODULE_CMD_FINI calls config_cfdata_detach() } which attempts to detach each device instance. If a detach fails, then } config_cfdata_detach fails, and the unload will fail. Does this mean that you'll end up with some device instances detached and not others? Nope. All non-zero units are prevented from unload. And unit zero is permitted only if there are no references, ie no clones. I don't see why non-zero units are special, can you explain? Open zero open 1, close 0, close 1. Should the close 1 unload? christos !DSPAM:52dc4fde23811308416388! - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
On Jan 19, 3:04pm, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | The mere existence of a non-zero unit is a "reference" that needs to | prevent unloading. What if it is the last reference? Then the ref count will go to zero after close without unloading, and you'll never unload. | The two checks (unit# and ref-count) are equivalent and redundant, and | only one of them needs to be there. Well, just keep the refcount one then. christos
Re: CVS commit: src/sys/opencrypto
On Sun, 19 Jan 2014, Christos Zoulas wrote: On Jan 19, 3:04pm, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/opencrypto | The mere existence of a non-zero unit is a "reference" that needs to | prevent unloading. What if it is the last reference? Then the ref count will go to zero after close without unloading, and you'll never unload. | The two checks (unit# and ref-count) are equivalent and redundant, and | only one of them needs to be there. Well, just keep the refcount one then. Yup - that's what I committed. The device_t wasn't available anyway, in crypto_modcmd() :) - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
re: CVS commit: src/sys/opencrypto
> Log Message: > Implement in-module ref-counting, and do not allow auto-unload if there > are existing references. > > Note that manual unloading is not prevented. > > OK christos@ > > XXX Also note that there is still a small window where the ref-count can > XXX be decremented, and then the process/thread preempted. If auto-unload > XXX happens before that thread can return from the module's code, bad > XXX things (tm) could happen. in this case, please simply disallow unload for this module always. if the race is fixed, it can be enabled again. .mrg.
Re: CVS commit: src/sys/opencrypto
In article <7458.1390534...@splode.eterna.com.au>, matthew green wrote: > >> Log Message: >> Implement in-module ref-counting, and do not allow auto-unload if there >> are existing references. >> >> Note that manual unloading is not prevented. >> >> OK christos@ >> >> XXX Also note that there is still a small window where the ref-count can >> XXX be decremented, and then the process/thread preempted. If auto-unload >> XXX happens before that thread can return from the module's code, bad >> XXX things (tm) could happen. > >in this case, please simply disallow unload for this module always. >if the race is fixed, it can be enabled again. I think that most module unloads suffer from this race, any ideas how to fix it? christos
Re: CVS commit: src/sys/opencrypto
Some modules get auto-loaded by the syscall mechanism. We already have a mechanism to determine if any lwp's are currently executing these syscalls, and if yes we prevent auto-unload. For device-driver modules, there is currently no equivalent mechanism. They get auto-loaded from code in specfs/vfsops as a result of trying to access the block/char device. We would need to specify exactly what constitutes a "reference" and then keep track. Is it only "open" that creates a reference? Someone mentioned that it might be possible to read/write without opening the device... Also need to consider what happens when the device is cloned ... There is a refcount in the struct module, and a generic module_hold() / module_rele() mechanism to manipulate. But it is currently used only for tracking inter-module references (ie, dependencies) as far as I can see. It could be used here, but as currently implemented (linear search for module name) it's probably too expensive. I'm not sure how we would handle "miscellaneous" modules... :) On Fri, 24 Jan 2014, Christos Zoulas wrote: In article <7458.1390534...@splode.eterna.com.au>, matthew green wrote: Log Message: Implement in-module ref-counting, and do not allow auto-unload if there are existing references. Note that manual unloading is not prevented. OK christos@ XXX Also note that there is still a small window where the ref-count can XXX be decremented, and then the process/thread preempted. If auto-unload XXX happens before that thread can return from the module's code, bad XXX things (tm) could happen. in this case, please simply disallow unload for this module always. if the race is fixed, it can be enabled again. I think that most module unloads suffer from this race, any ideas how to fix it? christos !DSPAM:52e2a490250671524318036! - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
Date: Fri, 24 Jan 2014 17:35:41 + (UTC) From: chris...@astron.com (Christos Zoulas) In article <7458.1390534...@splode.eterna.com.au>, matthew green wrote: > >> Log Message: >> XXX Also note that there is still a small window where the ref-count can >> XXX be decremented, and then the process/thread preempted. If auto-unload >> XXX happens before that thread can return from the module's code, bad >> XXX things (tm) could happen. > >in this case, please simply disallow unload for this module always. >if the race is fixed, it can be enabled again. I think that most module unloads suffer from this race, any ideas how to fix it? Shouldn't devsw_detach or config_fini_component handle this? Why does the crypto device need special reference counting that other devices don't?
Re: CVS commit: src/sys/opencrypto
On Fri, 24 Jan 2014, Taylor R Campbell wrote: Shouldn't devsw_detach or config_fini_component handle this? Why does the crypto device need special reference counting that other devices don't? The crypto device isn't special in this regard. Pretty much all device driver modules need this sort of ref-counting. Without it, the race exists and at some future time something will have reused/overwritten the memory previously occupied by the module, with non-deterministic results. Crypto is special only in that it tried to do some clean-up during the detach. The pool_destroy() code correctly noticed that there were still some outstanding allocations that had not been returned. So we sort of "got lucky" here and found out about the problem immediately, rather than waiting for some non-deterministic future. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
re: CVS commit: src/sys/opencrypto
> >> Implement in-module ref-counting, and do not allow auto-unload if there > >> are existing references. > >> > >> Note that manual unloading is not prevented. > >> > >> OK christos@ > >> > >> XXX Also note that there is still a small window where the ref-count can > >> XXX be decremented, and then the process/thread preempted. If auto-unload > >> XXX happens before that thread can return from the module's code, bad > >> XXX things (tm) could happen. > > > >in this case, please simply disallow unload for this module always. > >if the race is fixed, it can be enabled again. > > I think that most module unloads suffer from this race, any ideas how to > fix it? phase one: disable auto-unload. phase two: ??? the rest left as an exercise for the reader. :-)
re: CVS commit: src/sys/opencrypto
Phase 1 has been done. On Sat, 25 Jan 2014, matthew green wrote: Implement in-module ref-counting, and do not allow auto-unload if there are existing references. Note that manual unloading is not prevented. OK christos@ XXX Also note that there is still a small window where the ref-count can XXX be decremented, and then the process/thread preempted. If auto-unload XXX happens before that thread can return from the module's code, bad XXX things (tm) could happen. in this case, please simply disallow unload for this module always. if the race is fixed, it can be enabled again. I think that most module unloads suffer from this race, any ideas how to fix it? phase one: disable auto-unload. phase two: ??? the rest left as an exercise for the reader. :-) !DSPAM:52e37018210366601841969! - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
re: CVS commit: src/sys/opencrypto
> > phase one: disable auto-unload. > Phase 1 has been done. for the whole kernel?
re: CVS commit: src/sys/opencrypto
On Sun, 26 Jan 2014, matthew green wrote: phase one: disable auto-unload. Phase 1 has been done. for the whole kernel? No. Only for this specific module. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
re: CVS commit: src/sys/opencrypto
> >>> phase one: disable auto-unload. > > > >> Phase 1 has been done. > > > > for the whole kernel? > > No. Only for this specific module. right - my suggestion was that since this problem affects a large class of modules, until that is fixed, we should disable auto unload globally. .mrg.
Re: CVS commit: src/sys/opencrypto
On Sun, Jan 26, 2014 at 11:04:31AM +1100, matthew green wrote: > > > >>> phase one: disable auto-unload. > > > > > >> Phase 1 has been done. > > > > > > for the whole kernel? > > > > No. Only for this specific module. > > right - my suggestion was that since this problem affects a > large class of modules, until that is fixed, we should > disable auto unload globally. Does almost everything get loaded and auto-unloaded at boot time? If so that that isn't really a good idea. It also sounds like manual unloads of drivers can happen when the device is open - and that is also bad. A manual unload probably isn't going to race with open or close though. Disallowing unload completely would be a pain when developing drivers. David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src/sys/opencrypto
Date: Sun, 26 Jan 2014 11:36:32 + From: David Laight It also sounds like manual unloads of drivers can happen when the device is open - and that is also bad. A manual unload probably isn't going to race with open or close though. Disallowing unload completely would be a pain when developing drivers. It seems to me that the crux of the problem is that devsw_detach doesn't fail if the device is still in use, because we do keep no books about which devsws are still in use, so there's nothing to stop you from unloading a device's module before you've given a its close routine a chance to clean up. Would it suffice to add an open count to struct bdevsw and struct cdevsw, to be maintained by {bdev,cdev}_{open,close}? FreeBSD has a fancier reference-counting API for their devsw, but I'm not sure why they need that. A lesser problem is that the steps to finalize a device driver module are complicated (attach/detach the devsw, the cfdriver, the cfattach, the cfdata) and different drivers do them in a different order and may or may not back out the same way (e.g., dtv(4) ignores devsw_detach failure), so we ought to formalize what the right way to do this is once we have a way that can work.
Re: CVS commit: src/sys/opencrypto
Taylor R Campbell wrote: It seems to me that the crux of the problem is that devsw_detach doesn't fail if the device is still in use, because we do keep no books about which devsws are still in use, so there's nothing to stop you from unloading a device's module before you've given a its close routine a chance to clean up. Would it suffice to add an open count to struct bdevsw and struct cdevsw, to be maintained by {bdev,cdev}_{open,close}? I think this would be sufficient. A lesser problem is that the steps to finalize a device driver module are complicated (attach/detach the devsw, the cfdriver, the cfattach, the cfdata) and different drivers do them in a different order and may or may not back out the same way (e.g., dtv(4) ignores devsw_detach failure), so we ought to formalize what the right way to do this is once we have a way that can work. That would definitely be appreciated. It would seem that config_{init,fini}_component() are intended to handle much/most of this, but these routines are not documented and not used universally. (see sys/kern/subr_autoconf.c) - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/opencrypto
Date: Mon, 27 Jan 2014 05:45:32 -0800 (PST) From: Paul Goyette Taylor R Campbell wrote: > Would it suffice to add an open count to struct bdevsw and struct > cdevsw, to be maintained by {bdev,cdev}_{open,close}? I think this would be sufficient. Minor snag: all the bdevsw/cdevsw structs are const, so it would have to be maintained in a separate table. > A lesser problem is that the steps to finalize a device driver module > are complicated (attach/detach the devsw, the cfdriver, the cfattach, > the cfdata) and different drivers do them in a different order and may > or may not back out the same way (e.g., dtv(4) ignores devsw_detach > failure), so we ought to formalize what the right way to do this is > once we have a way that can work. That would definitely be appreciated. It would seem that config_{init,fini}_component() are intended to handle much/most of this, but these routines are not documented and not used universally. (see sys/kern/subr_autoconf.c) It looks like MODULE_CMD_INIT should do (other initialization and then) config_init_component and then devsw_attach last, and MODULE_CMD_FINI should do devsw_detach first and then config_fini_component (and then other finalization now that none of the resources can be in use by the userland or by the kernel). That way, on attach, the device won't be visible to userland until it is hooked up to autoconf, and on detach, if the device is open in userland, devsw_detach will fail and prevent the other destruction. I believe devsw_detach and config_fini_component are idempotent, so if for some reason config_fini_component fails the first time around, it need not back out by doing devsw_attach again. On the other hand, that might make diagnosis of the unload failure difficult because userland can no longer talk to the device. Perhaps we ought to combine config_init_component and devsw_attach to avoid having to write this pile of code like I wrote for drm2: case MODULE_CMD_INIT: error = config_init_component(cfdriver_ioconf_drm, cfattach_ioconf_drm, cfdata_ioconf_drm); if (error) { aprint_error("drm: unable to init config component: %d\n", error); goto init_fail0; } error = devsw_attach("drm", NULL, &bmajor, &drm_cdevsw, &cmajor); if (error) { aprint_error("drm: unable to attach devsw: %d\n", error); goto init_fail1; } return 0; /* These had better not fail... */ init_fail2: __unused (void)devsw_detach(NULL, &drm_cdevsw); init_fail1: (void)config_fini_component(cfdriver_ioconf_drm, cfattach_ioconf_drm, cfdata_ioconf_drm); init_fail0: return error; case MODULE_CMD_FINI: error = devsw_detach(NULL, &drm_cdevsw); if (error) return error; error = config_fini_component(cfdriver_ioconf_drm, cfattach_ioconf_drm, cfdata_ioconf_drm); if (error) return error; return 0;
Re: CVS commit: src/sys/opencrypto
On Fri, 27 Nov 2015, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Sat Nov 28 03:40:43 UTC 2015 Modified Files: src/sys/opencrypto: crypto.c Log Message: fix the build Thanks, and sorry for the breakage. +--+--+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: CVS commit: src/sys/opencrypto
On Tue, Apr 18, 2017 at 05:05:06PM +, Maya Rashish wrote: > XXX surrounding code seems fishy > > @@ -759,7 +759,6 @@ swcr_compdec(struct cryptodesc *crd, con > if (result < crd->crd_len) { > adj = result - crd->crd_len; > if (outtype == CRYPTO_BUF_MBUF) { > - adj = result - crd->crd_len; > m_adj((struct mbuf *)buf, adj); > } > /* Don't adjust the iov_len, it breaks the kmem_free */ > calling m_adj with negative adj sounds possibly unintended