Re: recent sysctl changes
On Mar 7, 2014, at 8:32 AM, Andreas Gustafsson wrote: > Thor Lancelot Simon wrote: >>> An application could, for example, maintain a single, shared, >>> malloc'ed buffer that is reused for multiple sysctl() calls and only >>> resized on ENOMEM returns. IMO, this is allowed by the API, but with >>> your change, a read of a CTLTYPE_QUAD variable will return the wrong >>> result if the buffer happened to be left with a size of 4 by a >>> previous read of a CTLTYPE_INT variable. >> >> But such a program would already have been buggy -- you can't read >> CTLTYPE_QUAD >> into a buffer of size 4. > > You can *attempt* to read CTLTYPE_QUAD into a buffer of size 4, > get ENOMEM, increase the buffer size, and retry. That's a specious argument. If you know the type if QUAD, and you use less than a buffer size of 8, it's a bug (and you're an idiot). That's like saying I can supply a bogus address, get EFAULT, and try with a good address.
Re: recent sysctl changes
Thor Lancelot Simon wrote: > > An application could, for example, maintain a single, shared, > > malloc'ed buffer that is reused for multiple sysctl() calls and only > > resized on ENOMEM returns. IMO, this is allowed by the API, but with > > your change, a read of a CTLTYPE_QUAD variable will return the wrong > > result if the buffer happened to be left with a size of 4 by a > > previous read of a CTLTYPE_INT variable. > > But such a program would already have been buggy -- you can't read > CTLTYPE_QUAD > into a buffer of size 4. You can *attempt* to read CTLTYPE_QUAD into a buffer of size 4, get ENOMEM, increase the buffer size, and retry. -- Andreas Gustafsson, g...@netbsd.org
Re: recent sysctl changes
On Fri, Mar 07, 2014 at 10:49:54AM +0200, Andreas Gustafsson wrote: > > An application could, for example, maintain a single, shared, > malloc'ed buffer that is reused for multiple sysctl() calls and only > resized on ENOMEM returns. IMO, this is allowed by the API, but with > your change, a read of a CTLTYPE_QUAD variable will return the wrong > result if the buffer happened to be left with a size of 4 by a > previous read of a CTLTYPE_INT variable. But such a program would already have been buggy -- you can't read CTLTYPE_QUAD into a buffer of size 4. Thor
Re: recent sysctl changes
David Laight wrote: > > Both of these aspects of the API are now broken: reading a 4-byte > > CTLTYPE_INT variable now works for any buffer size >= 4 *except* 8, > > That wasn't the intent of the change. > The intent was that if the size was 8 then the code would return > a numeric value of size 8, otherwise the size would be chnaged to > 4 and/or ENOMEM (stupid errno choice) returned. I understand the intent; I'm talking about the unintended consequences. > > and attempting to read an 8-byte CTLTYPE_QUAD variable into a buffer > > of less than 8 bytes is now guaranteed to yield ENOMEM *except* if the > > buffer size happens to be 4. > > A request to read a CTLTYPE_QUAD variable into a buffer that is shorter > than 8 bytes has always been a programming error. An application could, for example, maintain a single, shared, malloc'ed buffer that is reused for multiple sysctl() calls and only resized on ENOMEM returns. IMO, this is allowed by the API, but with your change, a read of a CTLTYPE_QUAD variable will return the wrong result if the buffer happened to be left with a size of 4 by a previous read of a CTLTYPE_INT variable. > The intent of the change was to relax that is the length happened to be 4. > > > IMO, this behavior violates both the > > letter of the sysctl() man page and the principle of least astonishment. > > I'm not sure about the latter. > I run 'sysctl -a' and find the name of the sysctl I'm interested in. > The result is a small number so I pass the address and size of a integer > variable and then print the result. > (Or the value is rather large and I think it might exceed 2^31 so I > use an int64.) > The 'principle of least astonishment' would mean that I get the value > that 'sysctl -a' printed. If you use a C variable of a size different than the documented size of the sysctl variable in case, your code is incorrect, and the resulting undefined behavior should not astonish you. > On a BE system I have to be extremely careful with the return values > from sysctl() or I see garbage. Yes, you do have to be careful, and we should not encourage carelessness by making incorrect code work in more cases. > Note that code calling systctl() has to either know whether the value > it is expecting is a string, structure, or number, or use the API calls > that expose the kernel internals in order to find out. Not only that, but in the case of a number, it also has to know whether it's an int or a quad, or use the appropriate API calls to find out. > > Also, the work-around is ineffective in the case of a caller that > > allocates the buffer dynamically using the size given by an initial > > sysctl() call with oldp = NULL. > > Code that does that for a numeric value will be quite happy with > either a 32bit of 64bit result. Not if it is expecting the other one. -- Andreas Gustafsson, g...@netbsd.org
Re: Recent sysctl changes
On Thu, Mar 06, 2014 at 09:19:00AM +0200, Andreas Gustafsson wrote: > Thor Lancelot Simon wrote: > > I don't actually know of any code that hands over a "wrong-size" > > buffer and will therefore break, though. Do you? > > No, but I think we should aim for correctness, not just "works for me". Seconded. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: Recent sysctl changes
Thor Lancelot Simon wrote: > I don't actually know of any code that hands over a "wrong-size" > buffer and will therefore break, though. Do you? No, but I think we should aim for correctness, not just "works for me". -- Andreas Gustafsson, g...@netbsd.org
Re: Recent sysctl changes
On Wed, 5 Mar 2014, Andreas Gustafsson wrote: I posted this on source-changes-d earlier today, but Jeff Rizzo asked me to take it to tech-kern. On Feb 27, David Laight said: Module Name: src Committed By: dsl Date: Thu Feb 27 22:50:52 UTC 2014 Modified Files: src/sys/kern: kern_sysctl.c Log Message: Allow CTLTYPE_INT and CTLTYPE_QUAD to be read and written as either 4 or 8 byte values regardless of the type. 64bit writes to 32bit variables must be valid (signed) values. 32bit reads of large values return -1. Amongst other things this should fix libm's code that reads machdep.sse as a 32bit int, but I'd changed it to 64bit (to common up some code). To generate a diff of this commit: cvs rdiff -u -r1.246 -r1.247 src/sys/kern/kern_sysctl.c In private correspondence with David, I have objected to this commit as well as his earlier change to the type of the machdep.sse sysctl variable. Regrettably, David and I have not been able to reach an agreement, and he has requested that I ask for opinions on a public list. So... 1. I object to the earlier change of the following sysctl variables from type CTLTYPE_INT to type CTLTYPE_QUAD: machdep.fpu_present machdep.osfxsr machdep.sse machdep.sse2 machdep.biosbasemem machdep.biosextmem My understanding of David's rationale for changing them is that it facilitated sharing code with the machdep.xsave_features variable which actually needs to be of type CTLTYPE_QUAD. By my reckoning, the savings from this code sharing amount to eight lines of code. Changing the types of existing sysctl variables breaks both source and binary compatibility and should not be done lightly; that's why we have both both hw.physmem and hw.physmem64, for example. Now, the types of multiple variables have been incompatibly changed for no reason other than saving a few lines of code. I believe the harm caused by the incompatible type change far outweighs the cost of a few added lines of code, and would like the original types to be restored. I am attaching a patch to do so; it also changes the newly added variables machdep.fpu_save and machdep.fpu_save_size to CTLTYPE_INT, but if David really thinks those should be CTLTYPE_QUAD, I will not argue that point. 2. I also object to the change of kern_sysctl.c 1.247. This change attempts to work around the problems caused by the changes to the variable types by making sysctl() return different types depending on the value of the *oldlenp argument. IMO, this is a bad idea. The *oldlenp argument does *not* specify the size of the data type expected by the caller, but rather the size of a buffer. The sysctl() API allows the caller to pass a buffer larger than the variable being read, and conversely, guarantees that passing a buffer that is too small results in ENOMEM. Both of these aspects of the API are now broken: reading a 4-byte CTLTYPE_INT variable now works for any buffer size >= 4 *except* 8, and attempting to read an 8-byte CTLTYPE_QUAD variable into a buffer of less than 8 bytes is now guaranteed to yield ENOMEM *except* if the buffer size happens to be 4. IMO, this behavior violates both the letter of the sysctl() man page and the principle of least astonishment. Also, the work-around is ineffective in the case of a caller that allocates the buffer dynamically using the size given by an initial sysctl() call with oldp = NULL. If the original types of the sysctl variables are restored, this work-around will no longer serve a purpose, and I'm asking for it to be removed. Opinions? I agree on both counts. - | 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: Recent sysctl changes
On Wed, Mar 05, 2014 at 03:56:54PM -0500, Thor Lancelot Simon wrote: > On Wed, Mar 05, 2014 at 08:55:50PM +0200, Andreas Gustafsson wrote: > > > > 2. I also object to the change of kern_sysctl.c 1.247. > > > > This change attempts to work around the problems caused by the changes > > to the variable types by making sysctl() return different types > > depending on the value of the *oldlenp argument. > > As I recall, we considered this approach before creating hw.physmem64, > and decided it was just a little too cute. > > I don't actually know of any code that hands over a "wrong-size" > buffer and will therefore break, though. Do you? I agree the > possibility does exist. I actually wonder if the code should also support single byte reads for things like machdep.sse which are effectively booleans. Maybe we should also allow 1, 4 and 8 byte reads for items declared as booleans. IIRC one of the arm ABIs uses 4 byte booleans - bound to be a cause for confusion at some point. David -- David Laight: da...@l8s.co.uk
Re: Recent sysctl changes
On Wed, Mar 05, 2014 at 08:55:50PM +0200, Andreas Gustafsson wrote: > > 2. I also object to the change of kern_sysctl.c 1.247. > > This change attempts to work around the problems caused by the changes > to the variable types by making sysctl() return different types > depending on the value of the *oldlenp argument. As I recall, we considered this approach before creating hw.physmem64, and decided it was just a little too cute. I don't actually know of any code that hands over a "wrong-size" buffer and will therefore break, though. Do you? I agree the possibility does exist. Thor
Recent sysctl changes
I posted this on source-changes-d earlier today, but Jeff Rizzo asked
me to take it to tech-kern.
On Feb 27, David Laight said:
> Module Name: src
> Committed By: dsl
> Date: Thu Feb 27 22:50:52 UTC 2014
>
> Modified Files:
> src/sys/kern: kern_sysctl.c
>
> Log Message:
> Allow CTLTYPE_INT and CTLTYPE_QUAD to be read and written as either 4 or 8
> byte values regardless of the type.
> 64bit writes to 32bit variables must be valid (signed) values.
> 32bit reads of large values return -1.
> Amongst other things this should fix libm's code that reads machdep.sse
> as a 32bit int, but I'd changed it to 64bit (to common up some code).
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.246 -r1.247 src/sys/kern/kern_sysctl.c
In private correspondence with David, I have objected to this commit
as well as his earlier change to the type of the machdep.sse
sysctl variable.
Regrettably, David and I have not been able to reach an agreement, and
he has requested that I ask for opinions on a public list. So...
1. I object to the earlier change of the following sysctl variables
from type CTLTYPE_INT to type CTLTYPE_QUAD:
machdep.fpu_present
machdep.osfxsr
machdep.sse
machdep.sse2
machdep.biosbasemem
machdep.biosextmem
My understanding of David's rationale for changing them is that it
facilitated sharing code with the machdep.xsave_features variable
which actually needs to be of type CTLTYPE_QUAD. By my reckoning,
the savings from this code sharing amount to eight lines of code.
Changing the types of existing sysctl variables breaks both source
and binary compatibility and should not be done lightly; that's
why we have both both hw.physmem and hw.physmem64, for example.
Now, the types of multiple variables have been incompatibly changed
for no reason other than saving a few lines of code.
I believe the harm caused by the incompatible type change far
outweighs the cost of a few added lines of code, and would like
the original types to be restored. I am attaching a patch to do so;
it also changes the newly added variables machdep.fpu_save and
machdep.fpu_save_size to CTLTYPE_INT, but if David really thinks
those should be CTLTYPE_QUAD, I will not argue that point.
2. I also object to the change of kern_sysctl.c 1.247.
This change attempts to work around the problems caused by the changes
to the variable types by making sysctl() return different types
depending on the value of the *oldlenp argument.
IMO, this is a bad idea. The *oldlenp argument does *not* specify the
size of the data type expected by the caller, but rather the size of a
buffer. The sysctl() API allows the caller to pass a buffer larger
than the variable being read, and conversely, guarantees that passing
a buffer that is too small results in ENOMEM.
Both of these aspects of the API are now broken: reading a 4-byte
CTLTYPE_INT variable now works for any buffer size >= 4 *except* 8,
and attempting to read an 8-byte CTLTYPE_QUAD variable into a buffer
of less than 8 bytes is now guaranteed to yield ENOMEM *except* if the
buffer size happens to be 4. IMO, this behavior violates both the
letter of the sysctl() man page and the principle of least astonishment.
Also, the work-around is ineffective in the case of a caller that
allocates the buffer dynamically using the size given by an initial
sysctl() call with oldp = NULL.
If the original types of the sysctl variables are restored, this
work-around will no longer serve a purpose, and I'm asking for it
to be removed.
Opinions?
--
Andreas Gustafsson, g...@netbsd.org
Index: x86_machdep.c
===
RCS file: /cvsroot/src/sys/arch/x86/x86/x86_machdep.c,v
retrieving revision 1.63
diff -u -r1.63 x86_machdep.c
--- x86_machdep.c 23 Feb 2014 22:38:40 - 1.63
+++ x86_machdep.c 3 Mar 2014 18:41:06 -
@@ -1056,7 +1056,17 @@
}
static void
-const_sysctl(struct sysctllog **clog, const char *name, u_quad_t value, int
tag)
+const_sysctl_int(struct sysctllog **clog, const char *name, int value, int tag)
+{
+ sysctl_createv(clog, 0, NULL, NULL,
+ CTLFLAG_PERMANENT | CTLFLAG_IMMEDIATE,
+ CTLTYPE_INT, name, NULL,
+ NULL, (u_quad_t)value, NULL, 0,
+ CTL_MACHDEP, tag, CTL_EOL);
+}
+
+static void
+const_sysctl_quad(struct sysctllog **clog, const char *name, u_quad_t value,
int tag)
{
sysctl_createv(clog, 0, NULL, NULL,
CTLFLAG_PERMANENT | CTLFLAG_IMMEDIATE,
@@ -1115,17 +1125,17 @@
CTL_MACHDEP, CTL_CREATE, CTL_EOL);
/* None of these can ever change once the system has booted */
- const_sysctl(clog, "fpu_present", i386_fpu_present, CPU_FPU_PRESENT);
- const_sysctl(clog, "osfxsr", i386_use_fxsave, CPU_OSFXSR);
- const_sysctl(clog, "sse", i386_has_sse, CPU_SSE);
- const_sysctl(clog, "sse2", i386_has_
