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 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 pine.neb.4.64.1401191416060.22...@screamer.whooppee.com, Paul Goyette p...@whooppee.com 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 pine.neb.4.64.1401191416060.22...@screamer.whooppee.com, Paul Goyette p...@whooppee.com 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 | -
