On Mon, 3 Nov 2014, Julian Elischer wrote:
On 11/3/14, 3:46 PM, Mateusz Guzik wrote:
Author: mjg
Date: Mon Nov 3 07:46:51 2014
New Revision: 274017
URL: https://svnweb.freebsd.org/changeset/base/274017
Log:
Provide an on-stack temporary buffer for small ioctl requests.
I'm not sure I like this. We don't know how many more levels
of stack may be needed.
128 bytes is nothing. select() always used a 512-byte array of 2048
bits for similar purposes. poll() uses an array of 32 struct pollfd's
similarly. (The names of these seem to have rotted. I think select()
used to use the name smallbits, and this was a good name for an array
of bits. Now select() uses the name s_selbits for its small array of
bits, and poll() uses the name smallbits for its small array of non-bits).
The struct is subject to uncontrolled bloat, unlike the bit array which
is limited by its hard-coded 2048. struct pollfd actually has size just
8 on all arches, so 32 of it is small.
I know that machines are getting faster with more memory,
but the current move towards bloating out the
worries me. we started out with a single page of stack (SHARED
with the U-area!). I think we are now at several pages.. I forget, is it 8?
It is still only 2 plus 1 guard page on i386. KSTACK_PAGES is a bogus
option (any use of the option to change it breaks it when the options
header is not included). KSTACK_GUARD_PAGES isn't a bogus option, so
it can only be changed by editing the source code; then changing it only
breaks things previously compiled with the old value.
On amd64, it is 4 plus 1.
The depth of syscalls is indeed horrifying. Similar techniques were
used in Slowaris to blow out its register windows on Sparc many years
ago.
I'm open to being persuaded but I think we need to have a discussion
about stack usage. We used to say that anything greater that, say
64 bytes should probably be allocated.
The top level of a syscall should be allowed to use more like a full
page (leaving 1 page for everything else). Only the top level can
resonably control its usage.
Modified:
head/sys/kern/sys_generic.c
Excessive quoting not deleted. I will complain about style bugs here instead
of in a reply to the original mail.
Modified: head/sys/kern/sys_generic.c
==============================================================================
--- head/sys/kern/sys_generic.c Mon Nov 3 07:18:42 2014
(r274016)
+++ head/sys/kern/sys_generic.c Mon Nov 3 07:46:51 2014
(r274017)
@@ -649,6 +649,7 @@ sys_ioctl(struct thread *td, struct ioct
u_long com;
int arg, error;
u_int size;
+ u_char smalldata[128];
caddr_t data;
The bogus type caddr_t is still used.
if (uap->com > 0xffffffff) {
@@ -680,17 +681,18 @@ sys_ioctl(struct thread *td, struct ioct
arg = (intptr_t)uap->data;
Bogus cast. intptr_t is only valid on void *, but uap->data has the
bogus opaque type caddr_t.
data = (void *)&arg;
The cast is correct. Then it is assumed that the opaque type caddr_t is
a pointer. If 'data; had type void *, then no assumptions would be needed
here (assumptions and conversions would be needed later, since the KPI has
some caddr_t mistakes built in).
size = 0;
- } else
- data = malloc((u_long)size, M_IOCTLOPS, M_WAITOK);
+ } else {
+ if (size <= sizeof(smalldata))
+ data = smalldata;
+ else
+ data = malloc((u_long)size, M_IOCTLOPS,
M_WAITOK);
Style bugs:
- old: unnecessary cast to u_long. It was to support unprototyped code.
- new: line too long, by blind re-indentation.
+ }
} else
data = (void *)&uap->data;
Many more bogus casts like this.
if (com & IOC_IN) {
error = copyin(uap->data, data, (u_int)size);
A more bogus cast to u_int. It was to support unprotyped code, but has
been wrong for that for more than 20 years, since copyin()'s KPI was
changed to take a size_t.
- if (error) {
- if (size > 0)
- free(data, M_IOCTLOPS);
- return (error);
- }
+ if (error != 0)
+ goto out;
} else if (com & IOC_OUT) {
/*
* Zero the buffer so the user always
@@ -704,7 +706,8 @@ sys_ioctl(struct thread *td, struct ioct
if (error == 0 && (com & IOC_OUT))
error = copyout(data, uap->data, (u_int)size);
Another bogus cast for the copyin() family.
- if (size > 0)
+out:
+ if (size > 0 && data != (caddr_t)&smalldata)
free(data, M_IOCTLOPS);
'size' is unsigned, so (size > 0) should be written as (size != 0).
Using caddr_t requires another bogus cast here. &smalldata should
be written as &smalldata[0] or simply smalldata. The address of
the array is subtly different and not wanted here, but reduces to
the same in the explicit conversion to caddr_t, assuming that caddr_t
is char * like it is, or void *; otherwise, the subtle difference
might have an effect.
return (error);
}
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
