Re: CVS commit: src/sys/opencrypto

2017-04-18 Thread coypu
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


Re: CVS commit: src/sys/opencrypto

2015-11-27 Thread Paul Goyette

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

2014-01-27 Thread Paul Goyette

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

2014-01-26 Thread David Laight
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

2014-01-26 Thread Taylor R Campbell
   Date: Sun, 26 Jan 2014 11:36:32 +
   From: David Laight da...@l8s.co.uk

   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

2014-01-25 Thread matthew green

  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

2014-01-25 Thread Paul Goyette

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

2014-01-25 Thread matthew green

  phase one:  disable auto-unload.

 Phase 1 has been done.

for the whole kernel?


re: CVS commit: src/sys/opencrypto

2014-01-25 Thread Paul Goyette

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

2014-01-25 Thread matthew green

  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

2014-01-24 Thread Christos Zoulas
In article 7458.1390534...@splode.eterna.com.au,
matthew green  m...@eterna.com.au 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

2014-01-24 Thread Paul Goyette
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  m...@eterna.com.au 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

2014-01-24 Thread Taylor R Campbell
   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  m...@eterna.com.au 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

2014-01-24 Thread Paul Goyette

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

2014-01-23 Thread matthew green

 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

2014-01-19 Thread Paul Goyette

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

2014-01-19 Thread Christos Zoulas
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

2014-01-19 Thread Christos Zoulas
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

2014-01-19 Thread Paul Goyette

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

2014-01-19 Thread Christos Zoulas
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

2014-01-19 Thread Paul Goyette

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

2014-01-19 Thread Christos Zoulas
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

2014-01-19 Thread John Nemeth
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

2014-01-19 Thread Paul Goyette
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

2014-01-19 Thread Christos Zoulas
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

2014-01-19 Thread Paul Goyette

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  |
-