NetBSD Problem Report #43587
From Wolfgang.Stukenbrock@nagler-company.com Fri Jul 9 10:18:10 2010
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
by www.NetBSD.org (Postfix) with ESMTP id D195363BAB2
for <gnats-bugs@gnats.NetBSD.org>; Fri, 9 Jul 2010 10:18:10 +0000 (UTC)
Message-Id: <20100709101800.231C22E478F@s013.nagler-company.com>
Date: Fri, 9 Jul 2010 12:18:00 +0200 (CEST)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: stupid error message during boot from sysctl if no COMPAT40 in kernel
X-Send-Pr-Version: 3.95
>Number: 43587
>Category: bin
>Synopsis: stupid error message during boot from sysctl if no COMPAT40 in kernel
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: bin-bug-people
>State: closed
>Class: change-request
>Submitter-Id: net
>Arrival-Date: Fri Jul 09 10:20:00 +0000 2010
>Closed-Date: Sun Jul 31 23:44:25 +0000 2016
>Last-Modified: Sun Jul 31 23:44:25 +0000 2016
>Originator: Wolfgang Stukenbrock
>Release: NetBSD 5.0.2
>Organization:
Dr. Nagler & Company GmbH
>Environment:
System: NetBSD s013 5.0.2 NetBSD 5.0.2 (NSW-S013) #2: Fri Jul 9 11:02:34 CEST 2010 root@s013:/usr/src/sys/arch/amd64/compile/NSW-S013 amd64
Architecture: x86_64
Machine: amd64
>Description:
In /etc/sysctl.conf the line "kern.no_sa_support?=1" tries to disable KENR_SA from
the COMPAT40 as default for security reasons. This is OK.
But if the kernel is compiled without COMPAT40, the option KERN_SA is not defined
in the kernel build and this variable gets read-only with the value 1.
This conflicts with the intention of "?=" of sysctl. The "?=" assumes, that the variable
is not there and will ignore the line silently.
But the variable is present (with value "1"), so sysctl is gooing to set the variable.
Due to the fact that it is read-only, this fails with an error message during boot on the console.
>How-To-Repeat:
Compile a kernel without COMPAT40 support and boot it.
>Fix:
There are two ways to "fix" this problem.
1. do not create the kernel variable if KERN_SA is not defined.
2. keep sysctl from setting a variable to the same value with "?=" it has before.
I'm not shure about any effects of version 1.
So I would prefer version 2, but I don't know, if some other variables in the kernel
depends on the ability to set the same value again (with "?=") and trigger some actions.
As a workaround for now I ignore the error message on the console, because it sounds to
dangerous for me to comment out this security setting in /etc/sysctl.conf for the case
that another kernel with COMPAT40 gets build on the system.
(I'm shure no onw will remember the change to /etc/sysctl.conf in that case ....)
I've not the time to create a patch for version 2 at the moment. Perhaps in one or two
months if this report is still open. sorry.
>Release-Note:
>Audit-Trail:
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in
Date: Fri, 9 Jul 2010 13:30:46 +0200
--Nq2Wo0NMKNjxTN9z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Untested, but maybe something along the lines of the attached patch would do
it?
The idea is to avoid the "try changing the value" part if the old value
is what should be set anyway.
Martin
--Nq2Wo0NMKNjxTN9z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=patch
Index: sysctl.c
===================================================================
RCS file: /cvsroot/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.131
diff -u -p -r1.131 sysctl.c
--- sysctl.c 11 Apr 2010 01:52:10 -0000 1.131
+++ sysctl.c 9 Jul 2010 11:28:33 -0000
@@ -125,8 +125,8 @@ static const struct handlespec *findhand
static void canonicalize(const char *, char *);
static void purge_tree(struct sysctlnode *);
static void print_tree(int *, u_int, struct sysctlnode *, u_int, int);
-static void write_number(int *, u_int, struct sysctlnode *, char *);
-static void write_string(int *, u_int, struct sysctlnode *, char *);
+static void write_number(int *, u_int, struct sysctlnode *, char *, bool);
+static void write_string(int *, u_int, struct sysctlnode *, char *, bool);
static void display_number(const struct sysctlnode *, const char *,
const void *, size_t, int);
static void display_string(const struct sysctlnode *, const char *,
@@ -898,10 +898,10 @@ parse(char *l)
case CTLTYPE_INT:
case CTLTYPE_BOOL:
case CTLTYPE_QUAD:
- write_number(&name[0], namelen, node, value);
+ write_number(&name[0], namelen, node, value, optional);
break;
case CTLTYPE_STRING:
- write_string(&name[0], namelen, node, value);
+ write_string(&name[0], namelen, node, value, optional);
break;
case CTLTYPE_STRUCT:
/*
@@ -1697,7 +1697,8 @@ sysctlperror(const char *fmt, ...)
* ********************************************************************
*/
static void
-write_number(int *name, u_int namelen, struct sysctlnode *node, char *value)
+write_number(int *name, u_int namelen, struct sysctlnode *node, char *value,
+ bool optional)
{
u_int ii, io;
u_quad_t qi, qo;
@@ -1752,6 +1753,15 @@ write_number(int *name, u_int namelen, s
break;
}
+ if (optional) {
+ rc = sysctl(name, namelen, o, &so, NULL, 0);
+ if (rc == -1) {
+ sysctlerror(0);
+ return;
+ }
+ if (memcmp(&o, &i, so) == 0)
+ return;
+ }
rc = sysctl(name, namelen, o, &so, i, si);
if (rc == -1) {
sysctlerror(0);
@@ -1775,7 +1785,8 @@ write_number(int *name, u_int namelen, s
}
static void
-write_string(int *name, u_int namelen, struct sysctlnode *node, char *value)
+write_string(int *name, u_int namelen, struct sysctlnode *node, char *value,
+ bool optional)
{
char *i, *o;
size_t si, so;
@@ -1794,8 +1805,21 @@ write_string(int *name, u_int namelen, s
exit(1);
}
+ if (optional) {
+ rc = sysctl(name, namelen, o, &so, NULL, 0);
+ if (rc == -1) {
+ free(o);
+ sysctlerror(0);
+ return;
+ }
+ if (strcmp(o, i) == 0) {
+ free(o);
+ return;
+ }
+ }
rc = sysctl(name, namelen, o, &so, i, si);
if (rc == -1) {
+ free(o);
sysctlerror(0);
return;
}
--Nq2Wo0NMKNjxTN9z--
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
Date: Fri, 09 Jul 2010 18:49:41 +0700
Date: Fri, 9 Jul 2010 10:20:01 +0000 (UTC)
From: Wolfgang.Stukenbrock@nagler-company.com
Message-ID: <20100709102001.394E863BAB7@www.NetBSD.org>
Your #1 looks like a reasonable thing to do to me (sysctl nodes shouldn't
exist unless the thing they control exists as well).
| 2. keep sysctl from setting a variable to the same value with "?="
| it has before.
In addition, I'd also fix sysctl, but not as you suggest there.
Rather, just stop it issuing an error message when it attempts to
set a variable (using ?=) and finds that the variable exists but
cannot be set (for any reason, read-only being the most obvious).
That I think is totally in keeping with the intent of the ?= operator,
and allows the problem you observed to be "fixed" without needing to
worry about testing whether the value being assigned is the same as the
value there already (just doing the read to test that might cause side
effects in some weird case) now worrying whether the write might be a trigger
in addition to setting the value.
kre
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org,
Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in
Date: Fri, 09 Jul 2010 14:18:07 +0200
Hi,
not 100% OK, but if you replace the line
"if (memcmp(&o, &i, so) == 0)"
in write_number() with
"if (memcmp(o, i, so) == 0)"
it works.
o and i are of type void*, and it makes no sence to compare the pointers.
The string part has not been tested by me, but it looks good to me.
The missing free() you've added should go into the distribution in any case.
By the way: your version fo sysctl.c seems to be more recent than the
version in 5.0.2.
I've added and tested the patch to the 5.0.2 version.
I think this should go to the next available distribution.
Thanks.
W. Stukenbrock
Martin Husemann wrote:
> The following reply was made to PR bin/43587; it has been noted by GNATS.
>
> From: Martin Husemann <martin@duskware.de>
> To: gnats-bugs@NetBSD.org
> Cc:
> Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in
> Date: Fri, 9 Jul 2010 13:30:46 +0200
>
> --Nq2Wo0NMKNjxTN9z
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> Untested, but maybe something along the lines of the attached patch would do
> it?
>
> The idea is to avoid the "try changing the value" part if the old value
> is what should be set anyway.
>
> Martin
>
> --Nq2Wo0NMKNjxTN9z
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: attachment; filename=patch
>
> Index: sysctl.c
> ===================================================================
> RCS file: /cvsroot/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 sysctl.c
> --- sysctl.c 11 Apr 2010 01:52:10 -0000 1.131
> +++ sysctl.c 9 Jul 2010 11:28:33 -0000
> @@ -125,8 +125,8 @@ static const struct handlespec *findhand
> static void canonicalize(const char *, char *);
> static void purge_tree(struct sysctlnode *);
> static void print_tree(int *, u_int, struct sysctlnode *, u_int, int);
> -static void write_number(int *, u_int, struct sysctlnode *, char *);
> -static void write_string(int *, u_int, struct sysctlnode *, char *);
> +static void write_number(int *, u_int, struct sysctlnode *, char *, bool);
> +static void write_string(int *, u_int, struct sysctlnode *, char *, bool);
> static void display_number(const struct sysctlnode *, const char *,
> const void *, size_t, int);
> static void display_string(const struct sysctlnode *, const char *,
> @@ -898,10 +898,10 @@ parse(char *l)
> case CTLTYPE_INT:
> case CTLTYPE_BOOL:
> case CTLTYPE_QUAD:
> - write_number(&name[0], namelen, node, value);
> + write_number(&name[0], namelen, node, value, optional);
> break;
> case CTLTYPE_STRING:
> - write_string(&name[0], namelen, node, value);
> + write_string(&name[0], namelen, node, value, optional);
> break;
> case CTLTYPE_STRUCT:
> /*
> @@ -1697,7 +1697,8 @@ sysctlperror(const char *fmt, ...)
> * ********************************************************************
> */
> static void
> -write_number(int *name, u_int namelen, struct sysctlnode *node, char *value)
> +write_number(int *name, u_int namelen, struct sysctlnode *node, char *value,
> + bool optional)
> {
> u_int ii, io;
> u_quad_t qi, qo;
> @@ -1752,6 +1753,15 @@ write_number(int *name, u_int namelen, s
> break;
> }
>
> + if (optional) {
> + rc = sysctl(name, namelen, o, &so, NULL, 0);
> + if (rc == -1) {
> + sysctlerror(0);
> + return;
> + }
> + if (memcmp(&o, &i, so) == 0)
> + return;
> + }
> rc = sysctl(name, namelen, o, &so, i, si);
> if (rc == -1) {
> sysctlerror(0);
> @@ -1775,7 +1785,8 @@ write_number(int *name, u_int namelen, s
> }
>
> static void
> -write_string(int *name, u_int namelen, struct sysctlnode *node, char *value)
> +write_string(int *name, u_int namelen, struct sysctlnode *node, char *value,
> + bool optional)
> {
> char *i, *o;
> size_t si, so;
> @@ -1794,8 +1805,21 @@ write_string(int *name, u_int namelen, s
> exit(1);
> }
>
> + if (optional) {
> + rc = sysctl(name, namelen, o, &so, NULL, 0);
> + if (rc == -1) {
> + free(o);
> + sysctlerror(0);
> + return;
> + }
> + if (strcmp(o, i) == 0) {
> + free(o);
> + return;
> + }
> + }
> rc = sysctl(name, namelen, o, &so, i, si);
> if (rc == -1) {
> + free(o);
> sysctlerror(0);
> return;
> }
>
> --Nq2Wo0NMKNjxTN9z--
>
>
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org,
Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
Date: Fri, 09 Jul 2010 14:37:38 +0200
Hi Robert,
your aproach is good too.
Perhaps it is better to avoid an additional read of the old value in
case it mayy trigger some actions in the kernel.
remark: I'm not confirmed with the possibilities of the sysctl-interface
here. Perhaps this aspect is obsolete, because there may be no actions
inside of the kernel after a read.
But if some tries to set a value that does not fit into the variable
(e.g. string to long) shouldn't be silently ignored!
If it would be possible to catch exactly the read-only case of the
variable if the write fails, that should be the sollution.
W. Stukenbrock
Robert Elz wrote:
> The following reply was made to PR bin/43587; it has been noted by GNATS.
>
> From: Robert Elz <kre@munnari.OZ.AU>
> To: gnats-bugs@NetBSD.org
> Cc:
> Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
> Date: Fri, 09 Jul 2010 18:49:41 +0700
>
> Date: Fri, 9 Jul 2010 10:20:01 +0000 (UTC)
> From: Wolfgang.Stukenbrock@nagler-company.com
> Message-ID: <20100709102001.394E863BAB7@www.NetBSD.org>
>
> Your #1 looks like a reasonable thing to do to me (sysctl nodes shouldn't
> exist unless the thing they control exists as well).
>
> | 2. keep sysctl from setting a variable to the same value with "?="
> | it has before.
>
> In addition, I'd also fix sysctl, but not as you suggest there.
> Rather, just stop it issuing an error message when it attempts to
> set a variable (using ?=) and finds that the variable exists but
> cannot be set (for any reason, read-only being the most obvious).
>
> That I think is totally in keeping with the intent of the ?= operator,
> and allows the problem you observed to be "fixed" without needing to
> worry about testing whether the value being assigned is the same as the
> value there already (just doing the read to test that might cause side
> effects in some weird case) now worrying whether the write might be a trigger
> in addition to setting the value.
>
> kre
>
>
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
Date: Fri, 9 Jul 2010 14:53:11 +0200
On Fri, Jul 09, 2010 at 11:55:03AM +0000, Robert Elz wrote:
> That I think is totally in keeping with the intent of the ?= operator,
I suggested the test for the same value because I'd like to see a warning
if - for whatever reason - the values configured in sysctl.conf differ from
the ones in effect on the system.
The admin can edit the .conf file anyway, it's just a default setting trying
to be sane & quiet in most cases.
Martin
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
Date: Fri, 09 Jul 2010 20:30:51 +0700
Date: Fri, 9 Jul 2010 12:55:02 +0000 (UTC)
From: Martin Husemann <martin@duskware.de>
Message-ID: <20100709125502.3746463BAB2@www.NetBSD.org>
| I suggested the test for the same value because I'd like to see a warning
| if - for whatever reason - the values configured in sysctl.conf differ from
| the ones in effect on the system.
I understand what you're saying, but I don't think that makes sense here.
Ideally, what would have happened, is that the sysctl node simply wouldn't
exist, and in that case sysctl(8) (using ?=) would have been silent. No
warnings about nodes that couldnt be set/changed/... That is exactly as
is intended.
| The admin can edit the .conf file anyway,
Yes, exactly - that's the point, if this is a setting that you want to have
happen, then you should be using the '=' operator, not '?=' - then you'll get
warnings (errors, whatever you want to call them) if for any reason the
setting doesn't happen. That is as it should be.
The ?= settings are (primarily) for stuff NetBSD is distributing [Aside: I
should launch another diatribe here about yet another user config file now
being corrupted with distribution settings - but I won't - today] that should
apply when it makes sense for them to apply, and should b silently ignored
in any other case.
Wolfgang.Stukenbrock@nagler-company.com said:
| But if some tries to set a value that does not fit into the variable
| (e.g. string to long) shouldn't be silently ignored!
In sysctl(8) I don't think that's a problem it makes sense to worry
about - or not in the relevant place - the data type comes from the
sysctl mib, the code cannot be attempting to set the "wrong" type, nor
anything else like that.
If that kind of error was going to occur, and be detected, it would be
where the user's arg value is being converted from the argv[] string to
the appropriate data type to store, not when the store operation happens
itself.
But if you really wanted to allow for that kind of error occurring, then
instead of just ignoring the error, you could test errno and ignore the
error codes that just mean "not possible now" rather than "you botched it"
just in case one of the latter type ever did happen (from the man page
(sysctl(3)) it appears that ignoring errors when errno is any of ENOENT,
EOPNOTSUPP, or EPERM would probably be sufficient.
As another side issue, I did think it slightly odd that in a patch intended
to make sysctl quiet, it was reading the old value of the node (and like you,
I have no idea if that is ever going to be a problem in practice) and if
that failed, issuing an error message ...
kre
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
Date: Fri, 09 Jul 2010 20:50:39 +0700
For reference this is the (apologies, not even compile tested...) patch
I would make to handle the sysctl part of this problem, I think this
is entirely adequate.
Note: patch is for NetBSD current (this problem has about 0% chance of
ever getting fixed in the NetBSD 5.0 series - it isn't a serious enough
problem - the fix could get pulled up for eventual inclusion in 5.2
though - I'm pretty sure it will be too late for 5.1 already).
kre
--- sysctl.c 2010-04-12 15:59:19.000000000 +0700
+++ sysctl.c-fixed 2010-07-09 20:46:50.000000000 +0700
@@ -125,8 +125,8 @@
static void canonicalize(const char *, char *);
static void purge_tree(struct sysctlnode *);
static void print_tree(int *, u_int, struct sysctlnode *, u_int, int);
-static void write_number(int *, u_int, struct sysctlnode *, char *);
-static void write_string(int *, u_int, struct sysctlnode *, char *);
+static void write_number(int *, u_int, struct sysctlnode *, char *, bool);
+static void write_string(int *, u_int, struct sysctlnode *, char *, bool);
static void display_number(const struct sysctlnode *, const char *,
const void *, size_t, int);
static void display_string(const struct sysctlnode *, const char *,
@@ -898,10 +898,10 @@
case CTLTYPE_INT:
case CTLTYPE_BOOL:
case CTLTYPE_QUAD:
- write_number(&name[0], namelen, node, value);
+ write_number(&name[0], namelen, node, value, optional);
break;
case CTLTYPE_STRING:
- write_string(&name[0], namelen, node, value);
+ write_string(&name[0], namelen, node, value, optional);
break;
case CTLTYPE_STRUCT:
/*
@@ -1697,7 +1697,7 @@
* ********************************************************************
*/
static void
-write_number(int *name, u_int namelen, struct sysctlnode *node, char *value)
+write_number(int *name, u_int namelen, struct sysctlnode *node, char *value, bool opt)
{
u_int ii, io;
u_quad_t qi, qo;
@@ -1754,7 +1754,8 @@
rc = sysctl(name, namelen, o, &so, i, si);
if (rc == -1) {
- sysctlerror(0);
+ if (!opt)
+ sysctlerror(0);
return;
}
@@ -1775,7 +1776,7 @@
}
static void
-write_string(int *name, u_int namelen, struct sysctlnode *node, char *value)
+write_string(int *name, u_int namelen, struct sysctlnode *node, char *value, bool opt)
{
char *i, *o;
size_t si, so;
@@ -1796,7 +1797,9 @@
rc = sysctl(name, namelen, o, &so, i, si);
if (rc == -1) {
- sysctlerror(0);
+ if (!opt)
+ sysctlerror(0);
+ free(o);
return;
}
From: Wolfgang Stukenbrock <wolfgang.stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: bin/43587: stupid error message during boot from sysctl if no COMPAT40 in kernel
Date: Tue, 28 May 2013 10:56:26 +0200
Hi - it took some time to get here again for me ...
I'm just preparing the upgrade to 6.1 and had a look at this again.
Your patch to kill the error output for optional settings sounds like
the better aproach as reading and check, if already set.
There will be less side effects now and in the future.
But I think only the EPERM case printing should be avoided.
(Accedently EPERM can happen in three cases - according to the manual,
but this is still better than suppressing all other kind of errors too.)
So useing
if (!opt || errno != EPERM)
should be better than just
if (!opt)
in both cases.
Best regards.
W. Stukenbrock
From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/43587 CVS commit: src/sbin/sysctl
Date: Sun, 31 Jul 2016 23:30:28 +0000
Module Name: src
Committed By: dholland
Date: Sun Jul 31 23:30:28 UTC 2016
Modified Files:
src/sbin/sysctl: sysctl.c
Log Message:
Apply patch from PR 43587, mostly from martin and kre. When writing a
sysctl variable using ?= fails with EPERM, don't print an error
message.
Ideally setting a sysctl to the same value it already has should also
not fail regardless of permissions, but this would need to be done in
the kernel.
To generate a diff of this commit:
cvs rdiff -u -r1.157 -r1.158 src/sbin/sysctl/sysctl.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 31 Jul 2016 23:44:25 +0000
State-Changed-Why:
patch applied, thanks
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.51 2026/08/10 02:28:17 riastradh Exp $
$NetBSD: gnats_config.sh,v 1.10 2026/05/13 22:00:09 riastradh Exp $
Copyright © 1994-2026
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home