Re: CVS commit: src/sys/kern (kern_event.c)

2021-01-22 Thread Paul Goyette

On Fri, 22 Jan 2021, Paul Goyette wrote:


On Thu, 21 Jan 2021, Paul Goyette wrote:


Ooopppsss ignore me - looks like this was already fixed and my update
missed it.

I'll retry.


OK, I built and installed a new kernel+userland.

Most everything works, and syslogd seems to work fine (at least, it
no longer panics during startup).

HOWEVER, firefox seems to be badly broken.  Attempting to open certain
pages results in never-ending-hang, and nothing ever gets rendered.  I
can use the Stop-Reloading "X" button, and the "oscillating dot" load
indicator stops oscillating, but nothing ever happens.  At that point,
the tab is hung and cannot load any other page, not even pages that
loaded successfully previously!  I _can_ delete the tab, and opening a
new tab works.

Some of the "failing" pages are:

airnow.gov
gmail.com
www.prudential.com/login
www.myaccountviewonline.com/AccountView/Logon


Slight correction:  above I said "nothing ever happens" but while I've
been composing this Email a couple of the above pages seem to have made
some progress (although none of them have finished and stopped the
"oscillating dot").  So "ever" is at least 5 minutes or longer ...  :)


I don't know if the kern_event.c changes are responsible, but I haven't
seen anything else recently.


I reverted kern_event.c to rev 1.110 and firefox behaves correctly.  So
it's pretty fair bet that the subsequent kern_event.c changes are the
reason for the breakage.

PR kern/55946 has been filed.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern (kern_event.c)

2021-01-22 Thread Paul Goyette

On Thu, 21 Jan 2021, Paul Goyette wrote:


Ooopppsss ignore me - looks like this was already fixed and my update
missed it.

I'll retry.


OK, I built and installed a new kernel+userland.

Most everything works, and syslogd seems to work fine (at least, it
no longer panics during startup).

HOWEVER, firefox seems to be badly broken.  Attempting to open certain
pages results in never-ending-hang, and nothing ever gets rendered.  I
can use the Stop-Reloading "X" button, and the "oscillating dot" load
indicator stops oscillating, but nothing ever happens.  At that point,
the tab is hung and cannot load any other page, not even pages that
loaded successfully previously!  I _can_ delete the tab, and opening a
new tab works.

Some of the "failing" pages are:

airnow.gov
gmail.com
www.prudential.com/login
www.myaccountviewonline.com/AccountView/Logon


Slight correction:  above I said "nothing ever happens" but while I've
been composing this Email a couple of the above pages seem to have made
some progress (although none of them have finished and stopped the
"oscillating dot").  So "ever" is at least 5 minutes or longer ...  :)


I don't know if the kern_event.c changes are responsible, but I haven't
seen anything else recently.

FWIW, I'm running firefox 83.0 from pkgsrc, around 2020-12-08





On Thu, 21 Jan 2021, Paul Goyette wrote:


This change seems to break everything!  As soon as I try to start
syslogd I hit the panic() that you added

[  28.0253983] panic: kqueue_scan,1491: kq=0xdc13890bc4c0 
kq->kq_count(1) != count(0), nmarker=1


[  28.0253983] cpu0: Begin traceback...
[  28.0253983] vpanic() at netbsd:vpanic+0x156
[  28.0253983] snprintf() at netbsd:snprintf
[  28.0253983] kqueue_check() at netbsd:kqueue_check+0x183
[  28.0253983] kevent1() at netbsd:kevent1+0x49f
[  28.0253983] sys___kevent50() at netbsd:sys___kevent50+0x33
[  28.0253983] syscall() at netbsd:syscall+0x23e
[  28.0253983] --- syscall (number 435) ---
[  28.0253983] netbsd:syscall+0x23e:
[  28.0253983] cpu0: End traceback...
[  28.0253983] fatal breakpoint trap in supervisor mode
[  28.0253983] trap type 1 code 0 rip 0x8021f415 cs 0x8 rflags 
0x202 cr2 0x78742459e000 ilevel 0x8 rsp 0xa809281ebb50
[  28.0253983] curlwp 0xdc138aa46540 pid 1352.1352 lowest kstack 
0xa809281e72c0

Stopped in pid 1352.1352 (syslogd) at   netbsd:breakpoint+0x5:  leave

I have a full crash dump if you need any further info.


Module Name:src
Committed By:   jdolecek
Date:   Thu Jan 21 18:09:23 UTC 2021

Modified Files:
   src/sys/kern: kern_event.c

Log Message:
adjust kq_check() (enabled with DEBUG) to new reality - it's now perfectly
normal to have kq_count bigger than number of the linked entries
on the kqueue

PR kern/50094, problem pointed out by Chuck Silvers


To generate a diff of this commit:
cvs rdiff -u -r1.111 -r1.112 src/sys/kern/kern_event.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern (kern_event.c)

2021-01-21 Thread Paul Goyette

Ooopppsss ignore me - looks like this was already fixed and my update
missed it.

I'll retry.


On Thu, 21 Jan 2021, Paul Goyette wrote:


This change seems to break everything!  As soon as I try to start
syslogd I hit the panic() that you added

[  28.0253983] panic: kqueue_scan,1491: kq=0xdc13890bc4c0 kq->kq_count(1) 
!= count(0), nmarker=1


[  28.0253983] cpu0: Begin traceback...
[  28.0253983] vpanic() at netbsd:vpanic+0x156
[  28.0253983] snprintf() at netbsd:snprintf
[  28.0253983] kqueue_check() at netbsd:kqueue_check+0x183
[  28.0253983] kevent1() at netbsd:kevent1+0x49f
[  28.0253983] sys___kevent50() at netbsd:sys___kevent50+0x33
[  28.0253983] syscall() at netbsd:syscall+0x23e
[  28.0253983] --- syscall (number 435) ---
[  28.0253983] netbsd:syscall+0x23e:
[  28.0253983] cpu0: End traceback...
[  28.0253983] fatal breakpoint trap in supervisor mode
[  28.0253983] trap type 1 code 0 rip 0x8021f415 cs 0x8 rflags 0x202 
cr2 0x78742459e000 ilevel 0x8 rsp 0xa809281ebb50
[  28.0253983] curlwp 0xdc138aa46540 pid 1352.1352 lowest kstack 
0xa809281e72c0

Stopped in pid 1352.1352 (syslogd) at   netbsd:breakpoint+0x5:  leave

I have a full crash dump if you need any further info.


Module Name:src
Committed By:   jdolecek
Date:   Thu Jan 21 18:09:23 UTC 2021

Modified Files:
   src/sys/kern: kern_event.c

Log Message:
adjust kq_check() (enabled with DEBUG) to new reality - it's now perfectly
normal to have kq_count bigger than number of the linked entries
on the kqueue

PR kern/50094, problem pointed out by Chuck Silvers


To generate a diff of this commit:
cvs rdiff -u -r1.111 -r1.112 src/sys/kern/kern_event.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern (kern_event.c)

2021-01-21 Thread Paul Goyette

This change seems to break everything!  As soon as I try to start
syslogd I hit the panic() that you added

[  28.0253983] panic: kqueue_scan,1491: kq=0xdc13890bc4c0 kq->kq_count(1) 
!= count(0), nmarker=1

[  28.0253983] cpu0: Begin traceback...
[  28.0253983] vpanic() at netbsd:vpanic+0x156
[  28.0253983] snprintf() at netbsd:snprintf
[  28.0253983] kqueue_check() at netbsd:kqueue_check+0x183
[  28.0253983] kevent1() at netbsd:kevent1+0x49f
[  28.0253983] sys___kevent50() at netbsd:sys___kevent50+0x33
[  28.0253983] syscall() at netbsd:syscall+0x23e
[  28.0253983] --- syscall (number 435) ---
[  28.0253983] netbsd:syscall+0x23e:
[  28.0253983] cpu0: End traceback...
[  28.0253983] fatal breakpoint trap in supervisor mode
[  28.0253983] trap type 1 code 0 rip 0x8021f415 cs 0x8 rflags 0x202 
cr2 0x78742459e000 ilevel 0x8 rsp 0xa809281ebb50
[  28.0253983] curlwp 0xdc138aa46540 pid 1352.1352 lowest kstack 
0xa809281e72c0
Stopped in pid 1352.1352 (syslogd) at   netbsd:breakpoint+0x5:  leave

I have a full crash dump if you need any further info.


Module Name:src
Committed By:   jdolecek
Date:   Thu Jan 21 18:09:23 UTC 2021

Modified Files:
src/sys/kern: kern_event.c

Log Message:
adjust kq_check() (enabled with DEBUG) to new reality - it's now 
perfectly

normal to have kq_count bigger than number of the linked entries
on the kqueue

PR kern/50094, problem pointed out by Chuck Silvers


To generate a diff of this commit:
cvs rdiff -u -r1.111 -r1.112 src/sys/kern/kern_event.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern

2021-01-21 Thread Tom Spindler (moof)
I believe it's this change that's made my vbox image panic at the drop of
a hat; while it occasionally panics before it even hits single user, it
also consistently panics when starting syslogd (even from single-user):

panic: kqueue_scan,1491: kq=0xc779aeff6dc0 kq->kq_count(1) != count(0), 
nmarker=1

and the call chain is syscall -> sys___kevent50 -> kevent1 -> kqueue_check

it also causes a panic while trying to dump more than half the time,
but I have managed to get an image or two. ("panic: atastart: channel 0
busy, xfer not possible", if you're curious.)

On Wed, Jan 20, 2021 at 09:39:09PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Wed Jan 20 21:39:09 UTC 2021
>
> Modified Files:
>   src/sys/kern: kern_event.c
>
> Log Message:
> fix a race in kqueue_scan() - when multiple threads check the same
> kqueue, it could happen other thread seen empty kqueue while kevent
> was being checked for re-firing and re-queued
>
> make sure to keep retrying if there are outstanding kevents even
> if no kevent is found on first pass through the queue, and only
> drop the KN_QUEUED flag and kq_count when actually completely done
> with the kevent
>
> change is inspired by the FreeBSD in-flux handling, but without
> introducing the reference counting
>
> PR kern/50094 by Christof Meerwald
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.110 -r1.111 src/sys/kern/kern_event.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>



Re: CVS commit: src/sys/kern

2020-11-05 Thread Rin Okuyama

On 2020/11/05 2:45, Paul Goyette wrote:

On Wed, 4 Nov 2020, Rin Okuyama wrote:


On 2020/11/04 22:52, Paul Goyette wrote:

On Wed, 4 Nov 2020, Rin Okuyama wrote:


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


Oops, sorry, I meant ptrace_{init,fini}(). These functions are not called
at all since this commit, which forbids ptrace(2) for non-root users.


If the module is built-in (``options PTRACE'' selected in the config
file), then the module will already have been initialized.

If the module is not built-in, then a privileged user will need to
modload(8) the module.

Prior to this change, the built-in ptrace_common module was calling
the ptrace module's init/fini routine.  Quite likely ptrace_common
was built-in (due to inclusion of file-system PROCFS), so the init
was handled during init_main().  This change ensures that the ptrace
init/fini routines are called ONLY if the ptrace module itself (not
the ptrace_common) routine is built-in.

Please check to make sure that ``options PTRACE'' is included in
your kernel config.


Yes:

$ config -x netbsd.gdb | grep PTRACE
###> options    PTRACE  # Include ptrace(2) syscall
###> options    PTRACE_HOOKS    # Include ptrace hooks

The problem is that ptrace_{init,fini}() are not called from
ptrace_modcmd():

https://nxr.netbsd.org/xref/src/sys/kern/sys_ptrace.c#184

   184 static int
   185 ptrace_modcmd(modcmd_t cmd, void *arg)
   186 {
   187 int error;
   188
   189 switch (cmd) {
   190 case MODULE_CMD_INIT:
   191 error = syscall_establish(_netbsd, ptrace_syscalls);
   192 break;
   193 case MODULE_CMD_FINI:
   194 error = syscall_disestablish(_netbsd, ptrace_syscalls);
   195 break;
   196 default:
   197 error = ENOTTY;
   198 break;
   199 }
   200 return error;
   201 }


Yes that would be a problem.


Can you easily confirm that ktrace(2) is unusable for non-privileged
users on 9.99.75 kernel:

$ gdb echo
GNU gdb (GDB) 8.3
...
(gdb) b main
Breakpoint 1 at 0x950: file /usr/src/bin/echo/echo.c, line 58.
(gdb) r
Starting program: /bin/echo
warning: Could not trace the inferior process.
Error:
warning: ptrace: Operation not permitted
terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR'
[1]   Abort trap (core dumped) gdb echo

Also, ptrace_{init,fini} should be moved from sys_ptrace_common.c to
sys_ptrace.c, IMO.


I have some prior obligations, so I won't be able to look at this
until this evening.

Thanks for the detailed analysis.


Fixed. Thanks!

rin


Re: CVS commit: src/sys/kern

2020-11-04 Thread Paul Goyette

OK, this is my mistake.  When I change the calls in the ptrace_common
modcmd, I should also have renamed the functions (including their
entries in sys/ptrace.h).  I will commit this shortly, before I leave.

Thanks for the "recipe" for reproducing the problem - I will try it 
later when I return.



On Wed, 4 Nov 2020, Rin Okuyama wrote:


On 2020/11/04 22:52, Paul Goyette wrote:

On Wed, 4 Nov 2020, Rin Okuyama wrote:


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


Oops, sorry, I meant ptrace_{init,fini}(). These functions are not called
at all since this commit, which forbids ptrace(2) for non-root users.


If the module is built-in (``options PTRACE'' selected in the config
file), then the module will already have been initialized.

If the module is not built-in, then a privileged user will need to
modload(8) the module.

Prior to this change, the built-in ptrace_common module was calling
the ptrace module's init/fini routine.  Quite likely ptrace_common
was built-in (due to inclusion of file-system PROCFS), so the init
was handled during init_main().  This change ensures that the ptrace
init/fini routines are called ONLY if the ptrace module itself (not
the ptrace_common) routine is built-in.

Please check to make sure that ``options PTRACE'' is included in
your kernel config.


Yes:

$ config -x netbsd.gdb | grep PTRACE
###> optionsPTRACE  # Include ptrace(2) syscall
###> optionsPTRACE_HOOKS# Include ptrace hooks

The problem is that ptrace_{init,fini}() are not called from
ptrace_modcmd():

https://nxr.netbsd.org/xref/src/sys/kern/sys_ptrace.c#184

   184 static int
   185 ptrace_modcmd(modcmd_t cmd, void *arg)
   186 {
   187  int error;
   188
   189  switch (cmd) {
   190  case MODULE_CMD_INIT:
   191 		error = syscall_establish(_netbsd, 
ptrace_syscalls);

   192  break;
   193  case MODULE_CMD_FINI:
   194 		error = syscall_disestablish(_netbsd, 
ptrace_syscalls);

   195  break;
   196  default:
   197  error = ENOTTY;
   198  break;
   199  }
   200  return error;
   201 }

Can you easily confirm that ktrace(2) is unusable for non-privileged
users on 9.99.75 kernel:

$ gdb echo
GNU gdb (GDB) 8.3
...
(gdb) b main
Breakpoint 1 at 0x950: file /usr/src/bin/echo/echo.c, line 58.
(gdb) r
Starting program: /bin/echo
warning: Could not trace the inferior process.
Error:
warning: ptrace: Operation not permitted
terminate called after throwing an instance of 
'gdb_exception_RETURN_MASK_ERROR'

[1]   Abort trap (core dumped) gdb echo

Also, ptrace_{init,fini} should be moved from sys_ptrace_common.c to
sys_ptrace.c, IMO.

Thanks,
rin

!DSPAM:5fa2b869233318156490363!




++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern

2020-11-04 Thread Paul Goyette

On Wed, 4 Nov 2020, Rin Okuyama wrote:


On 2020/11/04 22:52, Paul Goyette wrote:

On Wed, 4 Nov 2020, Rin Okuyama wrote:


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


Oops, sorry, I meant ptrace_{init,fini}(). These functions are not called
at all since this commit, which forbids ptrace(2) for non-root users.


If the module is built-in (``options PTRACE'' selected in the config
file), then the module will already have been initialized.

If the module is not built-in, then a privileged user will need to
modload(8) the module.

Prior to this change, the built-in ptrace_common module was calling
the ptrace module's init/fini routine.  Quite likely ptrace_common
was built-in (due to inclusion of file-system PROCFS), so the init
was handled during init_main().  This change ensures that the ptrace
init/fini routines are called ONLY if the ptrace module itself (not
the ptrace_common) routine is built-in.

Please check to make sure that ``options PTRACE'' is included in
your kernel config.


Yes:

$ config -x netbsd.gdb | grep PTRACE
###> optionsPTRACE  # Include ptrace(2) syscall
###> optionsPTRACE_HOOKS# Include ptrace hooks

The problem is that ptrace_{init,fini}() are not called from
ptrace_modcmd():

https://nxr.netbsd.org/xref/src/sys/kern/sys_ptrace.c#184

   184 static int
   185 ptrace_modcmd(modcmd_t cmd, void *arg)
   186 {
   187  int error;
   188
   189  switch (cmd) {
   190  case MODULE_CMD_INIT:
   191 		error = syscall_establish(_netbsd, 
ptrace_syscalls);

   192  break;
   193  case MODULE_CMD_FINI:
   194 		error = syscall_disestablish(_netbsd, 
ptrace_syscalls);

   195  break;
   196  default:
   197  error = ENOTTY;
   198  break;
   199  }
   200  return error;
   201 }


Yes that would be a problem.


Can you easily confirm that ktrace(2) is unusable for non-privileged
users on 9.99.75 kernel:

$ gdb echo
GNU gdb (GDB) 8.3
...
(gdb) b main
Breakpoint 1 at 0x950: file /usr/src/bin/echo/echo.c, line 58.
(gdb) r
Starting program: /bin/echo
warning: Could not trace the inferior process.
Error:
warning: ptrace: Operation not permitted
terminate called after throwing an instance of 
'gdb_exception_RETURN_MASK_ERROR'

[1]   Abort trap (core dumped) gdb echo

Also, ptrace_{init,fini} should be moved from sys_ptrace_common.c to
sys_ptrace.c, IMO.


I have some prior obligations, so I won't be able to look at this
until this evening.

Thanks for the detailed analysis.



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern

2020-11-04 Thread Rin Okuyama

On 2020/11/04 22:52, Paul Goyette wrote:

On Wed, 4 Nov 2020, Rin Okuyama wrote:


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


Oops, sorry, I meant ptrace_{init,fini}(). These functions are not called
at all since this commit, which forbids ptrace(2) for non-root users.


If the module is built-in (``options PTRACE'' selected in the config
file), then the module will already have been initialized.

If the module is not built-in, then a privileged user will need to
modload(8) the module.

Prior to this change, the built-in ptrace_common module was calling
the ptrace module's init/fini routine.  Quite likely ptrace_common
was built-in (due to inclusion of file-system PROCFS), so the init
was handled during init_main().  This change ensures that the ptrace
init/fini routines are called ONLY if the ptrace module itself (not
the ptrace_common) routine is built-in.

Please check to make sure that ``options PTRACE'' is included in
your kernel config.


Yes:

$ config -x netbsd.gdb | grep PTRACE
###> optionsPTRACE  # Include ptrace(2) syscall
###> optionsPTRACE_HOOKS# Include ptrace hooks

The problem is that ptrace_{init,fini}() are not called from
ptrace_modcmd():

https://nxr.netbsd.org/xref/src/sys/kern/sys_ptrace.c#184

184 static int
185 ptrace_modcmd(modcmd_t cmd, void *arg)
186 {
187 int error;
188
189 switch (cmd) {
190 case MODULE_CMD_INIT:
191 error = syscall_establish(_netbsd, 
ptrace_syscalls);
192 break;
193 case MODULE_CMD_FINI:
194 error = syscall_disestablish(_netbsd, 
ptrace_syscalls);
195 break;
196 default:
197 error = ENOTTY;
198 break;
199 }
200 return error;
201 }

Can you easily confirm that ktrace(2) is unusable for non-privileged
users on 9.99.75 kernel:

$ gdb echo
GNU gdb (GDB) 8.3
...
(gdb) b main
Breakpoint 1 at 0x950: file /usr/src/bin/echo/echo.c, line 58.
(gdb) r
Starting program: /bin/echo
warning: Could not trace the inferior process.
Error:
warning: ptrace: Operation not permitted
terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR'
[1]   Abort trap (core dumped) gdb echo

Also, ptrace_{init,fini} should be moved from sys_ptrace_common.c to
sys_ptrace.c, IMO.

Thanks,
rin


Re: CVS commit: src/sys/kern

2020-11-04 Thread Paul Goyette

On Wed, 4 Nov 2020, Rin Okuyama wrote:


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


Oops, sorry, I meant ptrace_{init,fini}(). These functions are not called
at all since this commit, which forbids ptrace(2) for non-root users.


If the module is built-in (``options PTRACE'' selected in the config
file), then the module will already have been initialized.

If the module is not built-in, then a privileged user will need to
modload(8) the module.

Prior to this change, the built-in ptrace_common module was calling
the ptrace module's init/fini routine.  Quite likely ptrace_common
was built-in (due to inclusion of file-system PROCFS), so the init
was handled during init_main().  This change ensures that the ptrace
init/fini routines are called ONLY if the ptrace module itself (not
the ptrace_common) routine is built-in.

Please check to make sure that ``options PTRACE'' is included in
your kernel config.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+

Re: CVS commit: src/sys/kern

2020-11-04 Thread Rin Okuyama

On 2020/11/04 22:31, Paul Goyette wrote:

On Wed, 4 Nov 2020, Rin Okuyama wrote:


Hi,

On 2020/10/26 0:55, Paul Goyette wrote:

Module Name:    src
Committed By:    pgoyette
Date:    Sun Oct 25 15:55:37 UTC 2020

Modified Files:
src/sys/kern: sys_ptrace_common.c

Log Message:
ptrace_Common is a module unto itself.  Don't use the ptrace module's
init/fini routines.


To generate a diff of this commit:
cvs rdiff -u -r1.87 -r1.88 src/sys/kern/sys_ptrace_common.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


This commit makes ptrace(2) unusable for non-privileged users;
ptrace_common_{init,fini}() should be called from somewhere.


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


Oops, sorry, I meant ptrace_{init,fini}(). These functions are not called
at all since this commit, which forbids ptrace(2) for non-root users.

Thanks,
rin


Re: CVS commit: src/sys/kern

2020-11-04 Thread Paul Goyette

On Wed, 4 Nov 2020, Rin Okuyama wrote:


Hi,

On 2020/10/26 0:55, Paul Goyette wrote:

Module Name:src
Committed By:   pgoyette
Date:   Sun Oct 25 15:55:37 UTC 2020

Modified Files:
src/sys/kern: sys_ptrace_common.c

Log Message:
ptrace_Common is a module unto itself.  Don't use the ptrace module's
init/fini routines.


To generate a diff of this commit:
cvs rdiff -u -r1.87 -r1.88 src/sys/kern/sys_ptrace_common.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


This commit makes ptrace(2) unusable for non-privileged users;
ptrace_common_{init,fini}() should be called from somewhere.


ptrace_common_{init,fini} are called from the ptrace_common module's
modcmd routine in kern/sys_ptrace_common.c.  The modcmd routine in
turn is called at module initialization time.  In the case of a
built-in module, it will be called by module_init via init_main; if
the module is loaded (or auto-loaded) module_load will call the
modcmd routine.

The module will be built-in if either ``options PTRACE'' or ``file-
system PROCFS'' is set in the kernel configuration file.


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern

2020-11-03 Thread Rin Okuyama

Hi,

On 2020/10/26 0:55, Paul Goyette wrote:

Module Name:src
Committed By:   pgoyette
Date:   Sun Oct 25 15:55:37 UTC 2020

Modified Files:
src/sys/kern: sys_ptrace_common.c

Log Message:
ptrace_Common is a module unto itself.  Don't use the ptrace module's
init/fini routines.


To generate a diff of this commit:
cvs rdiff -u -r1.87 -r1.88 src/sys/kern/sys_ptrace_common.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


This commit makes ptrace(2) unusable for non-privileged users;
ptrace_common_{init,fini}() should be called from somewhere.

Thanks,
rin


Re: CVS commit: src/sys/kern

2020-10-19 Thread Christos Zoulas
In article <20201019144701.a6d3bf...@cvs.netbsd.org>,
Kamil Rytarowski  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  kamil
>Date:  Mon Oct 19 14:47:01 UTC 2020
>
>Modified Files:
>   src/sys/kern: sys_ptrace.c sys_ptrace_common.c
>
>Log Message:
>Remove obsolete references to 4.4BSD papers

This removes the ptrace module code. Please diff before committing!

christos



re: CVS commit: src/sys/kern

2020-10-08 Thread David H. Gutteridge

On Mon, 07 Sep 2020 at 20:47:25 +1000, matthew green wrote:

"Jason R Thorpe" writes:

Module Name:src
Committed By:   thorpej
Date:   Mon Sep  7 03:50:41 UTC 2020

Modified Files:
src/sys/kern: files.kern init_main.c

Log Message:
Add the ability to set an alternate cnmagic in the kernel config
file, e.g.:

optionsCNMAGIC="\"+\""


thanks!  i need this for my er4 that some how does do break properly..

options(4) update?


I just added an entry for this to options(4). The bare bones, anyway.

(It seems that DDB_BREAK_CHAR is only used in one place now, that being
src/sys/arch/arm/sa11x0/sa11x0_com.c. I'm not sure if another detail to
contextualize DDB_BREAK_CHAR vs. CNMAGIC would be warranted?)

Dave


re: CVS commit: src/sys/kern

2020-09-07 Thread matthew green
"Jason R Thorpe" writes:
> Module Name:  src
> Committed By: thorpej
> Date: Mon Sep  7 03:50:41 UTC 2020
> 
> Modified Files:
>   src/sys/kern: files.kern init_main.c
> 
> Log Message:
> Add the ability to set an alternate cnmagic in the kernel config
> file, e.g.:
> 
> optionsCNMAGIC="\"+\""

thanks!  i need this for my er4 that some how does do break properly..

options(4) update?


Re: CVS commit: src/sys/kern

2020-08-02 Thread Kamil Rytarowski
On 02.08.2020 17:50, Taylor R Campbell wrote:
>> Date: Sun, 2 Aug 2020 17:35:06 +0200
>> From: Kamil Rytarowski 
>>
>> On 02.08.2020 16:44, Taylor R Campbell wrote:
 Date: Sun, 2 Aug 2020 16:04:15 +0200
 From: Kamil Rytarowski 

 On 02.08.2020 15:57, Taylor R Campbell wrote:
> But it sounds like the original motivation is that it triggered
> -Wvla...which frankly strikes me as a compiler bug since there's
> obviously no actual VLA created in sizeof; as far as I can tell
> there's no semantic difference between sizeof(device_t[n]) and
> sizeof(device_t) * n.

 This is not true:
>>>
>>> Which part of what I said are you claiming is not true, and what are
>>> you illustrating with the example program below?
>>
>> Calling it a compiler bug.
>>
>> Clang behaves the same way.
>>
>> $ clang -Wvla test.c
>> test.c:6:37: warning: variable length array used [-Wvla]
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>>^
>> 1 warning generated.
>>
>> Creating VLA is not needed for using it as an intermediate. In practice
>> in most/all cases it is optimized and actual VLA is not allocated.
> 
> This is not a matter of optimization in practice.  It's absolutely not
> an `intermediate' at all -- there is no VLA created, period, any more
> than sizeof(malloc(1)) causes any actual call to malloc or sizeof(*p)
> causes any dereference of a pointer.
> 
> This makes -Wvla less useful as a tool, because apparently it flags
> code that unquestionably does not have any bad effects of VLAs.  This
> happens because -Wvla is dumb -- it just looks for the syntax, not for
> the semantics of creating VLAs.  That's why I call it a bug -- it
> raises a false positive that makes it less useful.
> 

Compilers warns about VLA using ("variable length array used"), not
creating. There is no distinct compiler warning to discriminate VLA
creating from usage.

Anyway, code just using VLA is not that frequent, avoiding it is rather
simple and VLA can be avoided altogether (at least for non-external).



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2020-08-02 Thread Taylor R Campbell
> Date: Sun, 2 Aug 2020 17:35:06 +0200
> From: Kamil Rytarowski 
> 
> On 02.08.2020 16:44, Taylor R Campbell wrote:
> >> Date: Sun, 2 Aug 2020 16:04:15 +0200
> >> From: Kamil Rytarowski 
> >>
> >> On 02.08.2020 15:57, Taylor R Campbell wrote:
> >>> But it sounds like the original motivation is that it triggered
> >>> -Wvla...which frankly strikes me as a compiler bug since there's
> >>> obviously no actual VLA created in sizeof; as far as I can tell
> >>> there's no semantic difference between sizeof(device_t[n]) and
> >>> sizeof(device_t) * n.
> >>
> >> This is not true:
> > 
> > Which part of what I said are you claiming is not true, and what are
> > you illustrating with the example program below?
> 
> Calling it a compiler bug.
> 
> Clang behaves the same way.
> 
> $ clang -Wvla test.c
> test.c:6:37: warning: variable length array used [-Wvla]
> printf("sizeof = %zu\n", sizeof(int[argc]));
>^
> 1 warning generated.
> 
> Creating VLA is not needed for using it as an intermediate. In practice
> in most/all cases it is optimized and actual VLA is not allocated.

This is not a matter of optimization in practice.  It's absolutely not
an `intermediate' at all -- there is no VLA created, period, any more
than sizeof(malloc(1)) causes any actual call to malloc or sizeof(*p)
causes any dereference of a pointer.

This makes -Wvla less useful as a tool, because apparently it flags
code that unquestionably does not have any bad effects of VLAs.  This
happens because -Wvla is dumb -- it just looks for the syntax, not for
the semantics of creating VLAs.  That's why I call it a bug -- it
raises a false positive that makes it less useful.


Re: CVS commit: src/sys/kern

2020-08-02 Thread Kamil Rytarowski
On 02.08.2020 16:44, Taylor R Campbell wrote:
>> Date: Sun, 2 Aug 2020 16:04:15 +0200
>> From: Kamil Rytarowski 
>>
>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>> But it sounds like the original motivation is that it triggered
>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>> obviously no actual VLA created in sizeof; as far as I can tell
>>> there's no semantic difference between sizeof(device_t[n]) and
>>> sizeof(device_t) * n.
>>
>> This is not true:
> 
> Which part of what I said are you claiming is not true, and what are
> you illustrating with the example program below?
> 

Calling it a compiler bug.

Clang behaves the same way.

$ clang -Wvla test.c
test.c:6:37: warning: variable length array used [-Wvla]
printf("sizeof = %zu\n", sizeof(int[argc]));
   ^
1 warning generated.

Creating VLA is not needed for using it as an intermediate. In practice
in most/all cases it is optimized and actual VLA is not allocated.

> It seems to illustrate that sizeof(int[argc]) does exactly what one
> would expect it to do -- return the size of an argc-length array of
> ints, just like sizeof(int) * argc does.
> 
> 

The result is the same and I find the change as beneficial.

> 
>> #include 
>>
>> int
>> main(int argc, char **argv)
>> {
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>> return 0;
>> }
>>
>> $ ./a.out
>>
>> sizeof = 4
>> $ ./a.out 12 3
>> sizeof = 12
>> $ ./a.out 12 3 45 6
>> sizeof = 20




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2020-08-02 Thread Jaromír Doleček
Le dim. 2 août 2020 à 15:57, Taylor R Campbell
 a écrit :
> Why does it improve readability?

Certainly using cringe language features does not help readability.

> What else does -Wvla choke on in src/sys?

Some drm drivers, and uvm, particularly uvm_bio.c. I'd like to work
towards enabling -Wvla in kernel (i.e. remove all VLAs, same as Linux
kernel did in 2018), but I have some other stuff I want to finish
before I pick a new project.

Jaromir


Re: CVS commit: src/sys/kern

2020-08-02 Thread Taylor R Campbell
> Date: Sun, 2 Aug 2020 16:04:15 +0200
> From: Kamil Rytarowski 
> 
> On 02.08.2020 15:57, Taylor R Campbell wrote:
> > But it sounds like the original motivation is that it triggered
> > -Wvla...which frankly strikes me as a compiler bug since there's
> > obviously no actual VLA created in sizeof; as far as I can tell
> > there's no semantic difference between sizeof(device_t[n]) and
> > sizeof(device_t) * n.
> 
> This is not true:

Which part of what I said are you claiming is not true, and what are
you illustrating with the example program below?

It seems to illustrate that sizeof(int[argc]) does exactly what one
would expect it to do -- return the size of an argc-length array of
ints, just like sizeof(int) * argc does.



> #include 
> 
> int
> main(int argc, char **argv)
> {
> printf("sizeof = %zu\n", sizeof(int[argc]));
> return 0;
> }
> 
> $ ./a.out
> 
> sizeof = 4
> $ ./a.out 12 3
> sizeof = 12
> $ ./a.out 12 3 45 6
> sizeof = 20


Re: CVS commit: src/sys/kern

2020-08-02 Thread Joerg Sonnenberger
On Sun, Aug 02, 2020 at 01:57:11PM +, Taylor R Campbell wrote:
> But it sounds like the original motivation is that it triggered
> -Wvla...which frankly strikes me as a compiler bug since there's
> obviously no actual VLA created in sizeof; as far as I can tell
> there's no semantic difference between sizeof(device_t[n]) and
> sizeof(device_t) * n.

Yes, it seems strange to trigger a VLA warning in code that it is
required to be compile-time evaluated.

Joerg


Re: CVS commit: src/sys/kern

2020-08-02 Thread Kamil Rytarowski
On 02.08.2020 16:25, Paul Goyette wrote:
> On Sun, 2 Aug 2020, Kamil Rytarowski wrote:
> 
>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>> But it sounds like the original motivation is that it triggered
>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>> obviously no actual VLA created in sizeof; as far as I can tell
>>> there's no semantic difference between sizeof(device_t[n]) and
>>> sizeof(device_t) * n.
>>>
>>
>> This is not true:
>>
>> #include 
>>
>> int
>> main(int argc, char **argv)
>> {
>>    printf("sizeof = %zu\n", sizeof(int[argc]));
>>    return 0;
>> }
>>
>> $ ./a.out
>>
>> sizeof = 4
>> $ ./a.out 12 3
>> sizeof = 12
>> $ ./a.out 12 3 45 6
>> sizeof = 20
> 
> Modifying your example slightly, I print both variations:
> 
> #include 
> 
> int
> main(int argc, char **argv)
> {
> printf("sizeof = %zu\t%zu\n", sizeof(int[argc]), sizeof(int) * argc);
> return 0;
> }
> speedy:paul {653} ./a.out
> sizeof = 4  4
> speedy:paul {654} ./a.out 12 3
> sizeof = 12 12
> speedy:paul {655} ./a.out 12 3 45 6
> sizeof = 20 20
> 
> 
> Looks the same to me!
> 

The result is the same, but one uses VLA (at least formally), other not.

> 
> ++--+---+
> | Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
> | (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
> | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
> ++--+---+




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2020-08-02 Thread Paul Goyette

On Sun, 2 Aug 2020, Kamil Rytarowski wrote:


On 02.08.2020 15:57, Taylor R Campbell wrote:

But it sounds like the original motivation is that it triggered
-Wvla...which frankly strikes me as a compiler bug since there's
obviously no actual VLA created in sizeof; as far as I can tell
there's no semantic difference between sizeof(device_t[n]) and
sizeof(device_t) * n.



This is not true:

#include 

int
main(int argc, char **argv)
{
   printf("sizeof = %zu\n", sizeof(int[argc]));
   return 0;
}

$ ./a.out

sizeof = 4
$ ./a.out 12 3
sizeof = 12
$ ./a.out 12 3 45 6
sizeof = 20


Modifying your example slightly, I print both variations:

#include 

int
main(int argc, char **argv)
{
printf("sizeof = %zu\t%zu\n", sizeof(int[argc]), sizeof(int) * argc);
return 0;
}
speedy:paul {653} ./a.out
sizeof = 4  4
speedy:paul {654} ./a.out 12 3
sizeof = 12 12
speedy:paul {655} ./a.out 12 3 45 6
sizeof = 20 20


Looks the same to me!


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern

2020-08-02 Thread Kamil Rytarowski
On 02.08.2020 15:57, Taylor R Campbell wrote:
> But it sounds like the original motivation is that it triggered
> -Wvla...which frankly strikes me as a compiler bug since there's
> obviously no actual VLA created in sizeof; as far as I can tell
> there's no semantic difference between sizeof(device_t[n]) and
> sizeof(device_t) * n.
> 

This is not true:

#include 

int
main(int argc, char **argv)
{
printf("sizeof = %zu\n", sizeof(int[argc]));
return 0;
}

$ ./a.out

sizeof = 4
$ ./a.out 12 3
sizeof = 12
$ ./a.out 12 3 45 6
sizeof = 20



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2020-08-02 Thread Taylor R Campbell
> Date: Sun, 2 Aug 2020 10:47:21 +0200
> From: Jarom�r Dole ek 
> 
> Readability first and foremost in this case.
> 
> I was exploring if I can disable VLAs for the kernel altogether, this
> can't be done for now. Nevertheless, this change looked like it would
> be useful to make anyway.

Why does it improve readability?  Surely if we want the size of an
array of device_t of length n it's more to the point to say
sizeof(device_t[n]) directly than to talk about multiplying
sizeof(device_t) by n.  If that were the only question I think the
change makes readability worse, personally!

But it sounds like the original motivation is that it triggered
-Wvla...which frankly strikes me as a compiler bug since there's
obviously no actual VLA created in sizeof; as far as I can tell
there's no semantic difference between sizeof(device_t[n]) and
sizeof(device_t) * n.

What else does -Wvla choke on in src/sys?


Re: CVS commit: src/sys/kern

2020-08-02 Thread Jaromír Doleček
Readability first and foremost in this case.

I was exploring if I can disable VLAs for the kernel altogether, this
can't be done for now. Nevertheless, this change looked like it would
be useful to make anyway.

Le dim. 2 août 2020 à 01:15, Taylor R Campbell
 a écrit :
>
> > Module Name:src
> > Committed By:   jdolecek
> > Date:   Sat Aug  1 11:18:26 UTC 2020
> >
> > Modified Files:
> > src/sys/kern: subr_autoconf.c
> >
> > Log Message:
> > avoid VLA for the sizeof() calculations
>
> Why?


Re: CVS commit: src/sys/kern

2020-08-01 Thread Taylor R Campbell
> Module Name:src
> Committed By:   jdolecek
> Date:   Sat Aug  1 11:18:26 UTC 2020
> 
> Modified Files:
> src/sys/kern: subr_autoconf.c
> 
> Log Message:
> avoid VLA for the sizeof() calculations

Why?


Re: [sctp fix] Re: CVS commit: src/sys/kern

2020-07-18 Thread Maxime Villard

Le 28/04/2020 à 09:16, Luke Mewburn a écrit :

On 20-04-26 18:15, Maxime Villard wrote:
   |  - There was no demonstrated use-case justifying importing it. In addition,
   |major OSes like Windows and macOS do not implement SCTP. There just is 
no
   |demand for SCTP on the market; and on NetBSD, proportionally even less.

SCTP is used in mobile telco environments; the control plane for
3G and 4G networks uses SCTP (or TCP as an option, but mostly SCTP).


That may be true. I am mostly aware of the multimedia use cases.


Le 01/05/2020 à 18:46, m...@netbsd.org a écrit :

We can setup an equivalence: put as much effort into the SCTP removal
proposal as there was for the SCTP introduction proposal.

Since SCTP was just dropped in src without any prior discussion, I don't
think we need any discussion for removing it.


That would be fair, yes.


Le 02/05/2020 à 13:55, m...@netbsd.org a écrit :

I'm sorry for picking on SCTP in particular. Apparently it was added
because it was listed in src/doc/roadmaps.networking (and it's still
listed there).


But this doesn't address your own point, does it. The one-liner in
src/doc/roadmaps/networking gives no explanation on why we would want SCTP
in the first place. It doesn't even say where the code was imported from.

This brings the question of who, exactly, made this list. Several of the
items on this wanted list are actively *not* wanted, as Christos noted in
five of his "Comment[christos]".

At this point there is no doubt that SCTP in the NetBSD kernel has no
future. The only viable option I see is usrsctp:

https://github.com/sctplab/usrsctp

A userland version of the code, but portable, and actively maintained.

While here, notice the crazy buffer overflows that were fixed in it and
are still present in the NetBSD SCTP kernel code... Adds to my point,
that the code is of extremely poor quality.

Maxime


Re: CVS commit: src/sys/kern

2020-06-01 Thread Rin Okuyama

On 2020/06/02 2:08, Joerg Sonnenberger wrote:

On Sun, May 31, 2020 at 11:24:20PM +, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Sun May 31 23:24:20 UTC 2020

Modified Files:
src/sys/kern: kern_timeout.c

Log Message:
Stop allocating buffers dynamically in a DDB session, in order not to
disturb on-going debugged state of kernel datastructures.


This breaks the build with clang as it uses a declared-but-not-defined
type callout_cpu.


Fixed. Sorry for breakage.

Thanks,
rin


Re: CVS commit: src/sys/kern

2020-06-01 Thread Joerg Sonnenberger
On Sun, May 31, 2020 at 11:24:20PM +, Rin Okuyama wrote:
> Module Name:  src
> Committed By: rin
> Date: Sun May 31 23:24:20 UTC 2020
> 
> Modified Files:
>   src/sys/kern: kern_timeout.c
> 
> Log Message:
> Stop allocating buffers dynamically in a DDB session, in order not to
> disturb on-going debugged state of kernel datastructures.

This breaks the build with clang as it uses a declared-but-not-defined
type callout_cpu.

Joerg


re: CVS commit: src/sys/kern

2020-05-31 Thread matthew green
"Rin Okuyama" writes:
> Module Name:  src
> Committed By: rin
> Date: Sun May 31 08:33:48 UTC 2020
> 
> Modified Files:
>   src/sys/kern: kern_timeout.c
> 
> Log Message:
> db_show_callout(): struct callout_cpu and cpu_info are too much for stack.
> 
> XXX
> DDB can be running in the interrupt context, e.g., when activated from
> console. Therefore, use kmem_intr_alloc(9) instead of kmem_alloc(9).
> 
> Frame size, e.g. for m68k, becomes:
> 9212 (oops!) --> 0

please don't use allocators from ddb.  inspection from ddb
shouldn't go change internal datastructures and rely upon
things that can break working where possible

allocate a single static in the bss to use?


.mrg.


Re: CVS commit: src/sys/kern

2020-05-31 Thread Jason Thorpe



> On May 31, 2020, at 1:33 AM, Rin Okuyama  wrote:
> 
> db_show_callout(): struct callout_cpu and cpu_info are too much for stack.
> 
> XXX
> DDB can be running in the interrupt context, e.g., when activated from
> console. Therefore, use kmem_intr_alloc(9) instead of kmem_alloc(9).
> 
> Frame size, e.g. for m68k, becomes:
>9212 (oops!) --> 0
> 

I'm not sure we want to be calling memory allocators from within ddb.  I think 
it would be better to simply allocate that space in the .bss segment -- ddb 
only runs on 1 CPU at a time.

-- thorpej



Re: CVS commit: src/sys/kern

2020-05-07 Thread Kamil Rytarowski
On 07.05.2020 07:46, Michael van Elst wrote:
> On Wed, May 06, 2020 at 12:39:21PM +0200, Kamil Rytarowski wrote:
> 
> Hi Kamil,
> 
>> If I revert the pipe(2) changes on top of NetBSD-current, the test is no
>> longer racy again.
> 
> I tried 9.99.60 with and without my bugfix. The test always failed
> after some time with exactly that error.
> 
> If the change really had an effect, there would be a use-after-free bug
> somewhere else.
> 
> 
> Greetings,
> 

Thanks, I will investigating further.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2020-05-06 Thread Michael van Elst
On Wed, May 06, 2020 at 12:39:21PM +0200, Kamil Rytarowski wrote:

Hi Kamil,

> If I revert the pipe(2) changes on top of NetBSD-current, the test is no
> longer racy again.

I tried 9.99.60 with and without my bugfix. The test always failed
after some time with exactly that error.

If the change really had an effect, there would be a use-after-free bug
somewhere else.


Greetings,
-- 
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."


Re: CVS commit: src/sys/kern

2020-05-06 Thread Kamil Rytarowski
This caused regressions in t_ptrace_wait* tests and random
hangs/timeouts/failures.

If I revert the pipe(2) changes on top of NetBSD-current, the test is no
longer racy again.

http://netbsd.org/~kamil/patch-00249-pipe-revert.txt

Reproducer:

cd /usr/tests/lib/libc/sys
./t_ptrace_waitpid  tracer_sysctl_lookup_without_duplicates


It fails in a non-deterministic number of iterations:

[ 126.7088900] sorry, pid 20803 was killed: orphaned traced process
failed: /usr/src/tests/lib/libc/sys/t_ptrace_topology_wait.h:191:
msg_read_child("tracer ready" " from parent " "parent_tracer",
_tracer, , sizeof(msg)) == 0: Undefined error: 0

With this patch it is easier to reproduce the race:

Index: t_ptrace_topology_wait.h
===
RCS file: /cvsroot/src/tests/lib/libc/sys/t_ptrace_topology_wait.h,v
retrieving revision 1.1
diff -u -r1.1 t_ptrace_topology_wait.h
--- t_ptrace_topology_wait.h5 May 2020 00:33:37 -   1.1
+++ t_ptrace_topology_wait.h6 May 2020 10:32:37 -
@@ -248,7 +248,7 @@
 ATF_TC(tracer_sysctl_lookup_without_duplicates);
 ATF_TC_HEAD(tracer_sysctl_lookup_without_duplicates, tc)
 {
-   atf_tc_set_md_var(tc, "timeout", "15");
+// atf_tc_set_md_var(tc, "timeout", "15");
atf_tc_set_md_var(tc, "descr",
"Assert that await_zombie() in attach1 always finds a single "
"process and no other error is reported");
@@ -269,11 +269,13 @@
start = time(NULL);
while (true) {
DPRINTF("Step: %lu\n", N);
+   if (N % 100 == 0)
+   printf("Step: %lu\n", N);
tracer_sees_terminaton_before_the_parent_raw(true, false,
 false);
end = time(NULL);
diff = difftime(end, start);
-   if (diff >= 5.0)
+   if (diff >= 30.0)
break;
++N;
}


Can you have a look?

On 26.04.2019 19:20, Michael van Elst wrote:
> Module Name:  src
> Committed By: mlelstv
> Date: Fri Apr 26 17:20:49 UTC 2019
> 
> Modified Files:
>   src/sys/kern: sys_pipe.c
> 
> Log Message:
> Clean up pipe structure before recycling it.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.146 -r1.147 src/sys/kern/sys_pipe.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 
> 
> Modified files:
> 
> Index: src/sys/kern/sys_pipe.c
> diff -u src/sys/kern/sys_pipe.c:1.146 src/sys/kern/sys_pipe.c:1.147
> --- src/sys/kern/sys_pipe.c:1.146 Sun Jun 10 17:54:51 2018
> +++ src/sys/kern/sys_pipe.c   Fri Apr 26 17:20:49 2019
> @@ -1,4 +1,4 @@
> -/*   $NetBSD: sys_pipe.c,v 1.146 2018/06/10 17:54:51 jdolecek Exp $  */
> +/*   $NetBSD: sys_pipe.c,v 1.147 2019/04/26 17:20:49 mlelstv Exp $   */
>  
>  /*-
>   * Copyright (c) 2003, 2007, 2008, 2009 The NetBSD Foundation, Inc.
> @@ -68,7 +68,7 @@
>   */
>  
>  #include 
> -__KERNEL_RCSID(0, "$NetBSD: sys_pipe.c,v 1.146 2018/06/10 17:54:51 jdolecek 
> Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: sys_pipe.c,v 1.147 2019/04/26 17:20:49 mlelstv 
> Exp $");
>  
>  #include 
>  #include 
> @@ -1331,6 +1331,8 @@ pipeclose(struct pipe *pipe)
>  free_resources:
>   pipe->pipe_pgid = 0;
>   pipe->pipe_state = PIPE_SIGNALR;
> + pipe->pipe_peer = NULL;
> + pipe->pipe_lock = NULL;
>   pipe_free_kmem(pipe);
>   if (pipe->pipe_kmem != 0) {
>   pool_cache_put(pipe_rd_cache, pipe);
> 




signature.asc
Description: OpenPGP digital signature


Re: [sctp fix] Re: CVS commit: src/sys/kern

2020-05-02 Thread maya
On Fri, May 01, 2020 at 04:46:36PM +, m...@netbsd.org wrote:
> We can setup an equivalence: put as much effort into the SCTP removal
> proposal as there was for the SCTP introduction proposal.
> 
> Since SCTP was just dropped in src without any prior discussion, I don't
> think we need any discussion for removing it.

I'm sorry for picking on SCTP in particular. Apparently it was added
because it was listed in src/doc/roadmaps.networking (and it's still
listed there).


Re: [sctp fix] Re: CVS commit: src/sys/kern

2020-05-01 Thread maya
We can setup an equivalence: put as much effort into the SCTP removal
proposal as there was for the SCTP introduction proposal.

Since SCTP was just dropped in src without any prior discussion, I don't
think we need any discussion for removing it.


Re: [sctp fix] Re: CVS commit: src/sys/kern

2020-04-28 Thread Luke Mewburn
On 20-04-26 18:15, Maxime Villard wrote:
  |  - There was no demonstrated use-case justifying importing it. In addition,
  |major OSes like Windows and macOS do not implement SCTP. There just is no
  |demand for SCTP on the market; and on NetBSD, proportionally even less.

SCTP is used in mobile telco environments; the control plane for
3G and 4G networks uses SCTP (or TCP as an option, but mostly SCTP).

NetBSD is a server OS; and could be viable in that market, which
currently is mostly Linux (AFAICT), which does have a viable SCTP stack.

It's understandable why macOS (a client OS) doesn't support SCTP.


Besides the rest of your arguments (which may be valid), this particular
one is not.


regards,
Luke.


Re: CVS commit: src/sys/kern

2020-04-26 Thread Jason Thorpe



> On Apr 26, 2020, at 2:04 PM, Michael van Elst  wrote:
> 
> Log Message:
> fix DIAGNOSTIC build

non-DIAGNOSTIC you mean?

-- thorpej



[sctp fix] Re: CVS commit: src/sys/kern

2020-04-26 Thread Maxime Villard
Le 26/04/2020 à 16:21, Jonathan A. Kollasch a écrit :
> Module Name:  src
> Committed By: jakllsch
> Date: Sun Apr 26 14:21:14 UTC 2020
> 
> Modified Files:
>   src/sys/kern: uipc_socket.c
> 
> Log Message:
> Implement SCTP bug fixes found by maxv@.
> 
> Adding these seems to improve the SCTP situation.

Yeah, thanks... I remember I had sent an email about these bugs, no one cared,
so I just put big XXXs and left the broken thing as-is. That was more than a
year ago.

I remember also pointing out at some point the severe deficiencies of the SCTP
code we have, with no one caring once again (not even me actually).

In its current state (that is, the state it has been in ever since it was
imported five years ago), our SCTP code is a near-perfect example of gigantic,
buggy and useless bloat. Specifically, as far as I remember:

 - Last I checked, SCTP occupies, in number of lines of code, half of our IPv4
   network stack. In other words, when it was imported, our kernel netinet stack
   suddenly doubled in size. I don't see how we will ever MP-ify all of that.

 - There was no demonstrated use-case justifying importing it. In addition,
   major OSes like Windows and macOS do not implement SCTP. There just is no
   demand for SCTP on the market; and on NetBSD, proportionally even less.

 - The code is of remarkably poor quality, with bugs in all directions, complex
   pieces of logic that seem rarely justified, and implementation errors like
   the pointer bugs you just fixed. The bloat and bugs are already evident as
   early as in the first twenty lines of code of the sctp_input() entry point.

 - IIRC the mbuf API usage is very poor (though I don't remember the specifics
   here; I must have kept paper notes somewhere).

 - It seems that no one is maintaining it? Nothing has improved over the last
   five years, and there are no apparent signs that this situation will ever
   change. There are piecemeal changes like yours and mine, to accommodate API
   changes and fix obvious bugs, and that's about it, as far as I can tell.

At one point I hesitated about doing a big cleanup of SCTP. But I concluded
that it is too structurally broken, and that fixing it is a waste of time;
rewriting it would be faster and would result in a better, more functional,
and easier to maintain code.

Overall I think this is an example of bad policy. It's good to have support for
new protocols, but at some point, we need to question whether landing 40K lines
of buggy yet critical kernel code out of nowhere with no one maintaining it and
little justification for the feature is a good thing to do.

CC'ing core@ in case they are interested in making a useful statement for once.

In all cases, this code is disabled by default, which is a good thing given
its state, but results in us not giving a damn about it.

Maxime


Re: CVS commit: src/sys/kern

2020-04-13 Thread Andrew Doran
On Mon, Apr 13, 2020 at 04:34:48PM +0100, Roy Marples wrote:
> On 13/04/2020 16:31, Andrew Doran wrote:
> > Hi Roy.
> > 
> > On Sat, Apr 11, 2020 at 02:13:06AM +0100, Roy Marples wrote:
> > > On 10/04/2020 23:34, Andrew Doran wrote:
> > > > Module Name:src
> > > > Committed By:   ad
> > > > Date:   Fri Apr 10 22:34:36 UTC 2020
> > > > 
> > > > Modified Files:
> > > > src/sys/kern: vfs_mount.c
> > > > 
> > > > Log Message:
> > > > vfs_mountroot(): don't needlessly grab a second reference to the root 
> > > > vnode
> > > > (the kernel never chdirs) nor a lock on it.
> > > 
> > > So the kernel chrooting to sysctl init.root is still ok?
> > 
> > How is that accomplished?  I couldn't find the code and don't recall seeing
> > it.
> 
> sysctl -w init.root=/altroot
> 
> See the ZFS Root ramdisk:
> https://nxr.netbsd.org/xref/src/distrib/common/zfsroot.rc#33
> 
> I tested kernel from yesterdays sources and it still works fine, so I guess
> nothing was needed here.

I had a look and it's init that chroots, not the kernel, so no issue.  The
kernel chrooting would be evil.

Andrew


Re: CVS commit: src/sys/kern

2020-04-13 Thread Roy Marples

On 13/04/2020 16:31, Andrew Doran wrote:

Hi Roy.

On Sat, Apr 11, 2020 at 02:13:06AM +0100, Roy Marples wrote:

On 10/04/2020 23:34, Andrew Doran wrote:

Module Name:src
Committed By:   ad
Date:   Fri Apr 10 22:34:36 UTC 2020

Modified Files:
src/sys/kern: vfs_mount.c

Log Message:
vfs_mountroot(): don't needlessly grab a second reference to the root vnode
(the kernel never chdirs) nor a lock on it.


So the kernel chrooting to sysctl init.root is still ok?


How is that accomplished?  I couldn't find the code and don't recall seeing
it.


sysctl -w init.root=/altroot

See the ZFS Root ramdisk:
https://nxr.netbsd.org/xref/src/distrib/common/zfsroot.rc#33

I tested kernel from yesterdays sources and it still works fine, so I guess 
nothing was needed here.


Roy


Re: CVS commit: src/sys/kern

2020-04-13 Thread Andrew Doran
Hi Roy.

On Sat, Apr 11, 2020 at 02:13:06AM +0100, Roy Marples wrote:
> On 10/04/2020 23:34, Andrew Doran wrote:
> > Module Name:src
> > Committed By:   ad
> > Date:   Fri Apr 10 22:34:36 UTC 2020
> > 
> > Modified Files:
> > src/sys/kern: vfs_mount.c
> > 
> > Log Message:
> > vfs_mountroot(): don't needlessly grab a second reference to the root vnode
> > (the kernel never chdirs) nor a lock on it.
> 
> So the kernel chrooting to sysctl init.root is still ok?

How is that accomplished?  I couldn't find the code and don't recall seeing
it.

Andrew


Re: CVS commit: src/sys/kern

2020-04-10 Thread Roy Marples

On 10/04/2020 23:34, Andrew Doran wrote:

Module Name:src
Committed By:   ad
Date:   Fri Apr 10 22:34:36 UTC 2020

Modified Files:
src/sys/kern: vfs_mount.c

Log Message:
vfs_mountroot(): don't needlessly grab a second reference to the root vnode
(the kernel never chdirs) nor a lock on it.


So the kernel chrooting to sysctl init.root is still ok?

Roy


Re: [vfs_cache] Re: CVS commit: src/sys/kern

2020-04-05 Thread Andrew Doran
On Sun, Mar 29, 2020 at 11:41:23AM +0200, Maxime Villard wrote:
> Le 23/03/2020 ? 21:02, Andrew Doran a ?crit?:
> > Module Name:src
> > Committed By:   ad
> > Date:   Mon Mar 23 20:02:14 UTC 2020
> > 
> > Modified Files:
> > src/sys/kern: vfs_cache.c
> > 
> > Log Message:
> > cache_remove(): remove from the vnode list first, so cache_revlookup()
> > doesn't try to re-activate an entry no longer on the LRU list.
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.133 -r1.134 src/sys/kern/vfs_cache.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> 
> It appears that your recent changes in vfs_cache.c have introduced a
> use-after-free. Booting KASAN gives:
> 
>   ASan: Unauthorized Access In 0x...: Addr 0x... [1 byte, read, 
> PoolUseAfterFree]
> 
> It seems that the problem is here:
> 
> 557   if (nameiop == CREATE && (cnflags & ISLASTCN) != 0) {
> 558   /*
> 559* Last component and we are preparing to create
> 560* the named object, so flush the negative cache
> 561* entry.
> 562*/
> 563   COUNT(ncs_badhits);
> 564   cache_remove(ncp, true);< HERE
> 565   hit = false;
> 566   } else {
> 567   COUNT(ncs_neghits);
> 568   SDT_PROBE(vfs, namecache, lookup, hit, dvp, name,
> 569   namelen, 0, 0);
> 570   /* found neg entry; vn is already null from above */
> 571   hit = true;
> 572   }
> 573   if (iswht_ret != NULL) {
> 574   /*
> 575* Restore the ISWHITEOUT flag saved earlier.
> 576*/
> 577   *iswht_ret = ncp->nc_whiteout;  <-- ouch
> 578   } else {
> 579   KASSERT(!ncp->nc_whiteout);  <-- ouch
> 580   }
> 
> cache_remove() frees 'ncp', and then 'ncp->nc_whiteout' is read.

Fixed with vfs_cache.c 1.136.  Thank you for bringing it to my attention.

Andrew


[vfs_cache] Re: CVS commit: src/sys/kern

2020-03-29 Thread Maxime Villard
Le 23/03/2020 à 21:02, Andrew Doran a écrit :
> Module Name:  src
> Committed By: ad
> Date: Mon Mar 23 20:02:14 UTC 2020
> 
> Modified Files:
>   src/sys/kern: vfs_cache.c
> 
> Log Message:
> cache_remove(): remove from the vnode list first, so cache_revlookup()
> doesn't try to re-activate an entry no longer on the LRU list.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.133 -r1.134 src/sys/kern/vfs_cache.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

It appears that your recent changes in vfs_cache.c have introduced a
use-after-free. Booting KASAN gives:

ASan: Unauthorized Access In 0x...: Addr 0x... [1 byte, read, 
PoolUseAfterFree]

It seems that the problem is here:

557 if (nameiop == CREATE && (cnflags & ISLASTCN) != 0) {
558 /*
559  * Last component and we are preparing to create
560  * the named object, so flush the negative cache
561  * entry.
562  */
563 COUNT(ncs_badhits);
564 cache_remove(ncp, true);< HERE
565 hit = false;
566 } else {
567 COUNT(ncs_neghits);
568 SDT_PROBE(vfs, namecache, lookup, hit, dvp, name,
569 namelen, 0, 0);
570 /* found neg entry; vn is already null from above */
571 hit = true;
572 }
573 if (iswht_ret != NULL) {
574 /*
575  * Restore the ISWHITEOUT flag saved earlier.
576  */
577 *iswht_ret = ncp->nc_whiteout;  <-- ouch
578 } else {
579 KASSERT(!ncp->nc_whiteout);  <-- ouch
580 }

cache_remove() frees 'ncp', and then 'ncp->nc_whiteout' is read.

Maxime


Re: CVS commit: src/sys/kern

2020-03-09 Thread Maxime Villard
Le 08/03/2020 à 21:41, Andrew Doran a écrit :
> On Sun, Mar 08, 2020 at 08:34:29AM +0100, Maxime Villard wrote:
>> Le 08/03/2020 ? 01:31, Andrew Doran a ?crit?:
>>> Module Name:src
>>> Committed By:   ad
>>> Date:   Sun Mar  8 00:31:19 UTC 2020
>>>
>>> Modified Files:
>>> src/sys/kern: subr_kmem.c
>>>
>>> Log Message:
>>> KMEM_SIZE: append the size_t to the allocated buffer, rather than
>>> prepending, so it doesn't screw up the alignment of the buffer.
>>>
>>> Reported-by: syzbot+c024c50570cccac51...@syzkaller.appspotmail.com
>>>
>>>
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.78 -r1.79 src/sys/kern/subr_kmem.c
>>>
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>
>> This is wrong. The point of KMEM_SIZE is to store at a reliable location
>> the size of the buffer, in order to then verify that the 'size' argument
>> given to kmem_free() is correct.
>>
>> Here, you are using that size argument to compute the location, which
>> breaks the whole point of the check.
> 
> Sure I understand the frustration, but it's still correct according to
> the parameters I set for it 10+ years ago, which were for it to be a
> quick-n-dirty diagnostic aid.

No, it is not. KMEM_SIZE has found real bugs. Now this useful feature has
been lessened just to shut a sanitizer which was pointing another issue.

Again, as said previously, the real bug here is that you are making a
caller assume specific alignment from an allocator that _does not_
guarantee this alignment. To fix that, either (1) revert the assumption or
(2) make it part of the allocator's contract to guarantee this alignment.

I don't have a preference on (1) or (2), but the current design is more a
hack than anything else, and the subsequent change in KMEM_SIZE is
definitely wrong.

>> Also it probably collides with the KASAN shadow.
> 
> Hmm, is that purely an alignment issue then, because it works in 8 byte
> cells?  Otherwise it sounds like this should not be enabled with KASAN at
> all.

Not sure what you mean, but KASAN definitely needs to be enabled on
kmem. I've checked the code, and actually it is fine regarding KASAN for
now, because the computation of the redzone doesn't take KMEM_SIZE into
account (and so isn't affected by the fact that the location changed).


Re: CVS commit: src/sys/kern

2020-03-08 Thread Andrew Doran
On Sun, Mar 08, 2020 at 08:34:29AM +0100, Maxime Villard wrote:
> Le 08/03/2020 ? 01:31, Andrew Doran a ?crit?:
> > Module Name:src
> > Committed By:   ad
> > Date:   Sun Mar  8 00:31:19 UTC 2020
> > 
> > Modified Files:
> > src/sys/kern: subr_kmem.c
> > 
> > Log Message:
> > KMEM_SIZE: append the size_t to the allocated buffer, rather than
> > prepending, so it doesn't screw up the alignment of the buffer.
> > 
> > Reported-by: syzbot+c024c50570cccac51...@syzkaller.appspotmail.com
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.78 -r1.79 src/sys/kern/subr_kmem.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> 
> This is wrong. The point of KMEM_SIZE is to store at a reliable location
> the size of the buffer, in order to then verify that the 'size' argument
> given to kmem_free() is correct.
> 
> Here, you are using that size argument to compute the location, which
> breaks the whole point of the check.

Sure I understand the frustration, but it's still correct according to
the parameters I set for it 10+ years ago, which were for it to be a
quick-n-dirty diagnostic aid.

> Also it probably collides with the KASAN shadow.

Hmm, is that purely an alignment issue then, because it works in 8 byte
cells?  Otherwise it sounds like this should not be enabled with KASAN at
all.

Andrew
 
> Please revert this change.
>
> As said previously, if cacheline alignment is expected this way, then it
> should clearly be part of the kmem contract, and documented to be so.


Re: CVS commit: src/sys/kern

2020-03-07 Thread Maxime Villard
Le 08/03/2020 à 01:31, Andrew Doran a écrit :
> Module Name:  src
> Committed By: ad
> Date: Sun Mar  8 00:31:19 UTC 2020
> 
> Modified Files:
>   src/sys/kern: subr_kmem.c
> 
> Log Message:
> KMEM_SIZE: append the size_t to the allocated buffer, rather than
> prepending, so it doesn't screw up the alignment of the buffer.
> 
> Reported-by: syzbot+c024c50570cccac51...@syzkaller.appspotmail.com
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.78 -r1.79 src/sys/kern/subr_kmem.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

This is wrong. The point of KMEM_SIZE is to store at a reliable location
the size of the buffer, in order to then verify that the 'size' argument
given to kmem_free() is correct.

Here, you are using that size argument to compute the location, which
breaks the whole point of the check.

Also it probably collides with the KASAN shadow.

Please revert this change.

As said previously, if cacheline alignment is expected this way, then it
should clearly be part of the kmem contract, and documented to be so.

Thanks


Re: CVS commit: src/sys/kern

2020-03-03 Thread Andrew Doran
On Tue, Mar 03, 2020 at 02:55:16PM -0500, Christos Zoulas wrote:

> Module Name:  src
> Committed By: christos
> Date: Tue Mar  3 19:55:16 UTC 2020
> 
> Modified Files:
>   src/sys/kern: vfs_syscalls.c
> 
> Log Message:
> don't skip the rdir check for the lazy case; breaks chroot df(1) hiding.

That's likely my mistake.  Thank you.

Andrew


Re: CVS commit: src/sys/kern

2020-03-02 Thread Taylor R Campbell
> Date: Tue, 3 Mar 2020 12:41:32 +0900
> From: Rin Okuyama 
> 
> On 2020/03/03 1:00, Taylor R Campbell wrote:
> > Include kern_crashme.c in non-DEBUG kernels.
> > 
> > This is useful for simulating crashes in production to test failover.
> 
> I like this.
> 
> Could you please send pullup request to netbsd-9, or can I?

Go for it!


Re: CVS commit: src/sys/kern

2020-03-02 Thread Rin Okuyama

Hi,

On 2020/03/03 1:00, Taylor R Campbell wrote:

Module Name:src
Committed By:   riastradh
Date:   Mon Mar  2 16:00:54 UTC 2020

Modified Files:
src/sys/kern: files.kern

Log Message:
Include kern_crashme.c in non-DEBUG kernels.

This is useful for simulating crashes in production to test failover.


To generate a diff of this commit:
cvs rdiff -u -r1.43 -r1.44 src/sys/kern/files.kern

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


I like this.

Could you please send pullup request to netbsd-9, or can I?

Thanks,
rin


Re: CVS commit: src/sys/kern

2020-02-25 Thread Kamil Rytarowski
On 24.02.2020 21:47, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Mon Feb 24 20:47:47 UTC 2020
> 
> Modified Files:
>   src/sys/kern: init_main.c
> 
> Log Message:
> move config_init_mi() call before vfsinit(), which can trigger loading
> of VFS modules
> 
> fixes crash with LOCKDEBUG due to uninitialized mutex when zfs
> module is loaded in boot, because zfs's spa_init() calls config_mountroot()
> which now requires the config init having been done
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.521 -r1.522 src/sys/kern/init_main.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 
> 

kASan is still broken on boot. Please fix.


[   1.0188350] acpicpu1 at cpu1: ACPI CPU
[   1.0188350] cpu0 has 2 core siblings: cpu1 cpu0
[   1.0188350] cpu0 has 2 pkg siblings: cpu1 cpu0
[   1.0188350] cpu0 has 1 1st siblings: cpu0
[   1.0188350] cpu0 first in package: cpu0
[   1.0188350] cpu1 has 2 core siblings: cpu0 cpu1
[   1.0188350] cpu1 has 2 pkg siblings: cpu0 cpu1
[   1.0188350] cpu1 has 1 1st siblings: cpu0
[   1.0188350] cpu1 first in package: cpu0
[   1.2307575] panic: ASan: Unauthorized Access In 0x811e6be6:
Addr 0x98000f382b58 [8 bytes, read, PoolUseAfterFree]

[   1.2401020] cpu1: Begin traceback...
[   1.2501232] vpanic() at netbsd:vpanic+0x241
[   1.2701652] snprintf() at netbsd:snprintf
[   1.2902064] kasan_report() at netbsd:kasan_report+0x98
[   1.3102484] __asan_load8() at netbsd:__asan_load8+0x294
[   1.3302897] config_interrupts_thread() at
netbsd:config_interrupts_thread+0x68
[   1.3403126] cpu1: End traceback...
[   1.3403126] fatal breakpoint trap in supervisor mode
[   1.3503263] trap type 1 code 0 rip 0x8021e4b5 cs 0x8 rflags
0x246 cr2 0 ilevel 0 rsp 0x98017de07d60
[   1.3603479] curlwp 0x9800116a16c0 pid 0.30 lowest kstack
0x98017de002c0
Stopped in pid 0.30 (system) at netbsd:breakpoint+0x5:  leave
db{1}>

https://syzkaller.appspot.com/bug?id=aa6e0c00233b3e55340da80d7636bb2c18181e5f



signature.asc
Description: OpenPGP digital signature


re: CVS commit: src/sys/kern

2020-02-23 Thread matthew green
"Andrew Doran" writes:
> Module Name:  src
> Committed By: ad
> Date: Sun Feb 23 20:08:35 UTC 2020
> 
> Modified Files:
>   src/sys/kern: kern_pmf.c
> 
> Log Message:
> shutdown_all: take kernel_lock now that kern_reboot() doesn't.

ah, i am now guessing that having the kernel lock held
is why reboots hang and can't be broken into again.

we really need to have autoconf run out side of a spin
lock...  anyone want to work on this please?


.mrg.


re: CVS commit: src/sys/kern

2020-02-23 Thread matthew green
"Andrew Doran" writes:
> Module Name:  src
> Committed By: ad
> Date: Sun Feb 23 20:06:30 UTC 2020
> 
> Modified Files:
>   src/sys/kern: kern_reboot.c
> 
> Log Message:
> - If concurrent calls to kern_reboot(), only let the first do the deed.
> - Don't need kernel_lock for this (either OK, or suspendsched() called).

what happens if i enter ddb while rebooting, such that this
now sets 'rebooter' to some lwp.  inside ddb, i use 'mach cpu'
to change CPUs, which on some platforms actually swithces to
that CPU -- ie, we now have a different curlwp.

this while () will now hang forever, won't it?


.mrg.


Re: CVS commit: src/sys/kern

2020-02-21 Thread Kamil Rytarowski
On 20.02.2020 22:14, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Thu Feb 20 21:14:23 UTC 2020
> 
> Modified Files:
>   src/sys/kern: subr_autoconf.c
> 
> Log Message:
> protect deferred lists' manipulation by config_misc_lock, same as
> config_pending semaphore itself; right now this also covers
> DVF_ATTACH_INPROGRESS flag
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.265 -r1.266 src/sys/kern/subr_autoconf.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

After this commit kASan breaks on boot:

[   1.2418653] panic: ASan: Unauthorized Access In 0x811e0c46:
Addr 0xa7000f382a58 [8 bytes, read, PoolUseAfterFree]

[   1.2511909] cpu1: Begin traceback...
[   1.2612093] vpanic() at netbsd:vpanic+0x241 sys/kern/subr_prf.c:336
[   1.2812516] snprintf() at netbsd:snprintf
[   1.3012883] kasan_report() at netbsd:kasan_report+0x98
kasan_code_name sys/kern/subr_asan.c:186 [inline]
[   1.3012883] kasan_report() at netbsd:kasan_report+0x98
sys/kern/subr_asan.c:196
[   1.3213274] __asan_load8() at netbsd:__asan_load8+0x294
kasan_shadow_4byte_isvalid sys/kern/subr_asan.c:346 [inline]
[   1.3213274] __asan_load8() at netbsd:__asan_load8+0x294
kasan_shadow_8byte_isvalid sys/kern/subr_asan.c:360 [inline]
[   1.3213274] __asan_load8() at netbsd:__asan_load8+0x294
kasan_shadow_check sys/kern/subr_asan.c:412 [inline]
[   1.3213274] __asan_load8() at netbsd:__asan_load8+0x294
sys/kern/subr_asan.c:1182
[   1.3413734] config_interrupts_thread() at
netbsd:config_interrupts_thread+0x68 sys/kern/subr_autoconf.c:459
[   1.3513931] cpu1: End traceback...
[   1.3513931] fatal breakpoint trap in supervisor mode
[   1.3614094] trap type 1 code 0 rip 0x8021e4b5 cs 0x8 rflags
0x246 cr2 0 ilevel 0 rsp 0xa7017de07d60
[   1.3714294] curlwp 0xa700116a16c0 pid 0.30 lowest kstack
0xa7017de002c0
Stopped in pid 0.30 (system) at netbsd:breakpoint+0x5:  leave
db{1}>

https://syzkaller.appspot.com/bug?extid=1f0aefb06a387371fa14



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2020-02-18 Thread J. Hannken-Illjes
> On Feb 17, 2020, at 11:19 PM, Andrew Doran  wrote:
> 
> Hi,
> 
> On Thu, Feb 06, 2020 at 06:33:55PM +0100, J. Hannken-Illjes wrote:
> 
>>> On 12. Jan 2020, at 18:49, Andrew Doran  wrote:
>>> 
>>> Module Name:src
>>> Committed By:   ad
>>> Date:   Sun Jan 12 17:49:17 UTC 2020
>>> 
>>> Modified Files:
>>> src/sys/kern: vfs_vnode.c
>>> 
>>> Log Message:
>>> vput(): don't drop the vnode lock, carry the hold over into vrelel() which
>>> might need it anyway.
>>> 
>>> 
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.106 -r1.107 src/sys/kern/vfs_vnode.c
>>> 
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>> 
>> 
>> vput(vnode_t *vp)
>> {
>> +   int lktype;
>> 
>> -   VOP_UNLOCK(vp);
>> -   vrele(vp);
>> +   if ((vp->v_vflag & VV_LOCKSWORK) == 0) {
>> +   lktype = LK_EXCLUSIVE;
>> +   } else {
>> +   lktype = VOP_ISLOCKED(vp);
>> +   KASSERT(lktype != LK_NONE);
>> +   }
>> +   mutex_enter(vp->v_interlock);
>> +   vrelel(vp, 0, lktype);
>> }
>> 
>> This is quite wrong, from the manual:
>> 
>> VOP_ISLOCKED(vp)
>>  Test if the vnode vp is locked.  Possible return values are
>>  LK_EXCLUSIVE, LK_SHARED or 0 for lock held exclusively by the
>>  calling thread, shared lock held by anyone or unlocked,
>>  respectively.
>> 
>>  This function must never be used to make locking decisions at
>>  run time: it is provided only for diagnostic purposes.
>> 
>> I suppose you cannot determine if the current thread holds
>> a shared lock.
> 
> The intention of that last sentence was to dissuade people from doing error
> prone, complicated stuff with locks.  To my mind that idea of the locking
> primitives is to take something difficult (concurrency) and package it up in
> a way that's much easier to think about and work with - and yes it's still
> complicated.
> 
> There are limited cases when I think it makes sense to infer lock ownership,
> for example when known for sure that a RW lock is held but the type of hold
> needs to be known - like this case.  The failure case there is the lock
> being unheld, which would be caught by LOCKDEBUG in this case.  Consider
> that a rw_exit() with the lock unheld by the caller will produce what you
> might charitably call "undefined behaviour" in a non-LOCKDEBUG kernel.
> 
> Andrew

Yes, I think it is safe here but it deserves a comment like the text above.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)


signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/kern

2020-02-17 Thread Andrew Doran
Hi,

On Thu, Feb 06, 2020 at 06:33:55PM +0100, J. Hannken-Illjes wrote:

> > On 12. Jan 2020, at 18:49, Andrew Doran  wrote:
> > 
> > Module Name:src
> > Committed By:   ad
> > Date:   Sun Jan 12 17:49:17 UTC 2020
> > 
> > Modified Files:
> > src/sys/kern: vfs_vnode.c
> > 
> > Log Message:
> > vput(): don't drop the vnode lock, carry the hold over into vrelel() which
> > might need it anyway.
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.106 -r1.107 src/sys/kern/vfs_vnode.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> > 
> 
>  vput(vnode_t *vp)
>  {
> +   int lktype;
> 
> -   VOP_UNLOCK(vp);
> -   vrele(vp);
> +   if ((vp->v_vflag & VV_LOCKSWORK) == 0) {
> +   lktype = LK_EXCLUSIVE;
> +   } else {
> +   lktype = VOP_ISLOCKED(vp);
> +   KASSERT(lktype != LK_NONE);
> +   }
> +   mutex_enter(vp->v_interlock);
> +   vrelel(vp, 0, lktype);
>  }
> 
> This is quite wrong, from the manual:
> 
>  VOP_ISLOCKED(vp)
>   Test if the vnode vp is locked.  Possible return values are
>   LK_EXCLUSIVE, LK_SHARED or 0 for lock held exclusively by the
>   calling thread, shared lock held by anyone or unlocked,
>   respectively.
> 
>   This function must never be used to make locking decisions at
>   run time: it is provided only for diagnostic purposes.
> 
> I suppose you cannot determine if the current thread holds
> a shared lock.

The intention of that last sentence was to dissuade people from doing error
prone, complicated stuff with locks.  To my mind that idea of the locking
primitives is to take something difficult (concurrency) and package it up in
a way that's much easier to think about and work with - and yes it's still
complicated.

There are limited cases when I think it makes sense to infer lock ownership,
for example when known for sure that a RW lock is held but the type of hold
needs to be known - like this case.  The failure case there is the lock
being unheld, which would be caught by LOCKDEBUG in this case.  Consider
that a rw_exit() with the lock unheld by the caller will produce what you
might charitably call "undefined behaviour" in a non-LOCKDEBUG kernel.

Andrew


Re: CVS commit: src/sys/kern

2020-02-06 Thread J. Hannken-Illjes
> On 12. Jan 2020, at 18:49, Andrew Doran  wrote:
> 
> Module Name:  src
> Committed By: ad
> Date: Sun Jan 12 17:49:17 UTC 2020
> 
> Modified Files:
>   src/sys/kern: vfs_vnode.c
> 
> Log Message:
> vput(): don't drop the vnode lock, carry the hold over into vrelel() which
> might need it anyway.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.106 -r1.107 src/sys/kern/vfs_vnode.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

 vput(vnode_t *vp)
 {
+   int lktype;

-   VOP_UNLOCK(vp);
-   vrele(vp);
+   if ((vp->v_vflag & VV_LOCKSWORK) == 0) {
+   lktype = LK_EXCLUSIVE;
+   } else {
+   lktype = VOP_ISLOCKED(vp);
+   KASSERT(lktype != LK_NONE);
+   }
+   mutex_enter(vp->v_interlock);
+   vrelel(vp, 0, lktype);
 }

This is quite wrong, from the manual:

 VOP_ISLOCKED(vp)
  Test if the vnode vp is locked.  Possible return values are
  LK_EXCLUSIVE, LK_SHARED or 0 for lock held exclusively by the
  calling thread, shared lock held by anyone or unlocked,
  respectively.

  This function must never be used to make locking decisions at
  run time: it is provided only for diagnostic purposes.

I suppose you cannot determine if the current thread holds
a shared lock.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig


signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/kern

2020-01-21 Thread Simon Burge
"Christos Zoulas" wrote:

> Log Message:
>
> Don't crash if we are on a hippie trail, head full of zombie

+1 for any Australian references in a commit message :)

Cheers,
Simon.


re: CVS commit: src/sys/kern

2019-12-12 Thread matthew green
> Therefore, I wouldn't bother adding kcov.h headers, and rolling back to the
> previous version of in_interrupt for now is fine, considering that kcov
> currently has no use outside of amd64.

if you want to put code in sys/kern please make it portable.

adding more MD code in MI places is the opposite stance we
have been working towards since netbsd formed.


.mrg.


Re: CVS commit: src/sys/kern

2019-12-12 Thread Maxime Villard

Le 08/12/2019 à 14:22, Martin Husemann a écrit :

On Sun, Dec 08, 2019 at 12:58:20PM +0100, Maxime Villard wrote:

kMSan has special constraints which, in this specific case, come down to: each
function called from a KCOV instrumentation callback must be a static inline
tagged with __nomsan.

This was not the case with the updated in_interrupt(), but also still isn't the
case with the lwp_getspecific() call, which will have to be dropped.


This does not sound like a good reason to introduce MD code in sys/kern to
me. Could should not be made worse to deal with sanitizer restrictions.

Are there any alternatives?


The clean way of doing this is having an MD kcov.h included from subr_kcov.c,
like kASan.

However, I expect that we will want a per-lwp kcov flag to indicate the current
context (in exception, in interrupt, in nmi, etc), and when that happens, I
don't expect that we will need in_interrupt anymore.

Therefore, I wouldn't bother adding kcov.h headers, and rolling back to the
previous version of in_interrupt for now is fine, considering that kcov
currently has no use outside of amd64.


re: CVS commit: src/sys/kern

2019-12-10 Thread matthew green
> > > Log Message:
> > > Expunge the panicstr checks, we don't need them.
> > 
> > can you explain why?
> 
> Sure, [ .. ]

ah, wow.  i didn't realise it had such a bad effect on cpus before
they actually go properly down.

we should probably work hard to make them go down faster if possible,
though maybe that's already as good as it can be.

> > what sort of crash-time testing did you perform?
> 
> That the system can be debugged and reset cleanly.  If we've got code in DDB
> that hangs up or crashdups don't work then that's something we should fix.
> I've not run into any in a long time, they seem to get fixed.
> 
> Do you have a particular concern or something else in mind?

i was also curious about crash dumps working, but mostly
this change is about reducing true lossage during a crash.

thanks.


.mrg.


Re: CVS commit: src/sys/kern

2019-12-10 Thread Andrew Doran
On Wed, Dec 11, 2019 at 09:06:33AM +1100, matthew green wrote:

> "Andrew Doran" writes:
> > Module Name:src
> > Committed By:   ad
> > Date:   Mon Dec  9 21:02:10 UTC 2019
> > 
> > Modified Files:
> > src/sys/kern: kern_rwlock.c
> > 
> > Log Message:
> > Expunge the panicstr checks, we don't need them.
> 
> can you explain why?

Sure, I have developed a bit of a feel for it after years off watching the
thing panic.  The checks to not go too hard on the assertions once panicstr
are set are pretty good in my experience - we don't want that to snowball.

The ones that disable locking were of some kind of use (at least to me) back
in 2007 before we had a decent LOCKDEBUG setup for the newlock2 primitives. 
So it sprang out of development requirements and an uncertainty about all
how this new synchronization stuff was going to behave in practice more than
a desire to do the right thing.

On a uniprocessor or dual core box back then the panicstr checks didn't
really seem to have many bad effects, but with more CPUs it often seems to
make the crash much worse than it needs to be and I keep bumping into the
effects of it.  Here's a craptacular example:

http://www.netbsd.org/~ad/

That's kind of amusing to look at and it's only my framebuffer memory but I
worry that it could just as well be inodes or mbufs or anything else that
belongs to the user, and I think that until the CPUs are locked up and
activity stopped, the thing needs to keep working as properly as it can.

> what sort of crash-time testing did you perform?

That the system can be debugged and reset cleanly.  If we've got code in DDB
that hangs up or crashdups don't work then that's something we should fix.
I've not run into any in a long time, they seem to get fixed.

Do you have a particular concern or something else in mind?

Cheers,
Andrew


re: CVS commit: src/sys/kern

2019-12-10 Thread matthew green
"Andrew Doran" writes:
> Module Name:  src
> Committed By: ad
> Date: Mon Dec  9 21:02:10 UTC 2019
> 
> Modified Files:
>   src/sys/kern: kern_rwlock.c
> 
> Log Message:
> Expunge the panicstr checks, we don't need them.

can you explain why?  what sort of crash-time testing did
you perform?

thanks.


.mrg.


Re: CVS commit: src/sys/kern

2019-12-09 Thread Jason Thorpe


> On Dec 9, 2019, at 1:08 PM, Paul Goyette  wrote:
> 
> On Mon, 9 Dec 2019, Andrew Doran wrote:
> 
>> Module Name: src
>> Committed By:ad
>> Date:Mon Dec  9 21:05:23 UTC 2019
>> 
>> Modified Files:
>>  src/sys/kern: kern_mutex.c
>> 
>> Log Message:
>> - Add a mutex_owner_running() for the benefit of the pagedaemon, which
>> needs help with locking things in reverse order.
> 
> Should this be added to the mutex(9) man page?

Honestly, I think it should be protectively wrapped in something like 
__MUTEX_PRIVATE or some such, because it's not really something that things 
should be using except under very specialized circumstances.

-- thorpej



Re: CVS commit: src/sys/kern

2019-12-09 Thread Paul Goyette

On Mon, 9 Dec 2019, Andrew Doran wrote:


Module Name:src
Committed By:   ad
Date:   Mon Dec  9 21:05:23 UTC 2019

Modified Files:
src/sys/kern: kern_mutex.c

Log Message:
- Add a mutex_owner_running() for the benefit of the pagedaemon, which
 needs help with locking things in reverse order.


Should this be added to the mutex(9) man page?


++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/kern

2019-12-08 Thread Martin Husemann
On Sun, Dec 08, 2019 at 12:58:20PM +0100, Maxime Villard wrote:
> kMSan has special constraints which, in this specific case, come down to: each
> function called from a KCOV instrumentation callback must be a static inline
> tagged with __nomsan.
> 
> This was not the case with the updated in_interrupt(), but also still isn't 
> the
> case with the lwp_getspecific() call, which will have to be dropped.

This does not sound like a good reason to introduce MD code in sys/kern to
me. Could should not be made worse to deal with sanitizer restrictions.

Are there any alternatives?

Martin


Re: CVS commit: src/sys/kern

2019-12-08 Thread Maxime Villard

Le 08/12/2019 à 00:51, Kamil Rytarowski a écrit :

On 08.12.2019 00:35, matthew green wrote:

Module Name:src
Committed By:   kamil
Date:   Sat Dec  7 19:50:34 UTC 2019

Modified Files:
src/sys/kern: subr_kcov.c

Log Message:
Revert the in_interrupt() change to use again the x86 specific code

This is prerequisite for kMSan and upcoming kernel changes.

Discussed with 


why is this?  what is the problem?


kMSan has special constraints which, in this specific case, come down to: each
function called from a KCOV instrumentation callback must be a static inline
tagged with __nomsan.

This was not the case with the updated in_interrupt(), but also still isn't the
case with the lwp_getspecific() call, which will have to be dropped.


Re: CVS commit: src/sys/kern

2019-12-07 Thread Kamil Rytarowski
On 08.12.2019 00:35, matthew green wrote:
>> Module Name: src
>> Committed By:kamil
>> Date:Sat Dec  7 19:50:34 UTC 2019
>>
>> Modified Files:
>>  src/sys/kern: subr_kcov.c
>>
>> Log Message:
>> Revert the in_interrupt() change to use again the x86 specific code
>>
>> This is prerequisite for kMSan and upcoming kernel changes.
>>
>> Discussed with 
> 
> why is this?  what is the problem?
> 
> we shouldn't be putting back MD code into sys/kern files.
> the existing interface to check this *is* cpu_intr_p(),
> which should resolve to the same basic thing as the x86
> specific version.
> 
> 
> .mrg.
> 

I will let Maxime to answer.



signature.asc
Description: OpenPGP digital signature


re: CVS commit: src/sys/kern

2019-12-07 Thread matthew green
> Module Name:  src
> Committed By: kamil
> Date: Sat Dec  7 19:50:34 UTC 2019
> 
> Modified Files:
>   src/sys/kern: subr_kcov.c
> 
> Log Message:
> Revert the in_interrupt() change to use again the x86 specific code
> 
> This is prerequisite for kMSan and upcoming kernel changes.
> 
> Discussed with 

why is this?  what is the problem?

we shouldn't be putting back MD code into sys/kern files.
the existing interface to check this *is* cpu_intr_p(),
which should resolve to the same basic thing as the x86
specific version.


.mrg.


Re: CVS commit: src/sys/kern [change in kern_lwp.c]

2019-12-03 Thread Maxime Villard

Le 01/12/2019 à 16:27, Andrew Doran a écrit :

Module Name:src
Committed By:   ad
Date:   Sun Dec  1 15:27:58 UTC 2019

Modified Files:
src/sys/kern: kern_lwp.c

Log Message:
Fix a longstanding problem with LWP limits.  When changing the user's
LWP count, we must use the process credentials because that's what
the accounting entity is tied to.

Reported-by: syzbot+d193266676f635661...@syzkaller.appspotmail.com


Is there a mistake in the Reported-by? It corresponds to a report that was
closed as a malformed duplicate of this original report:


https://syzkaller.appspot.com/bug?id=8878f056a0a9cf0cc405f1926a4f236fdc721642

Also, the bug is still there apparently, if you want to give a look...


Re: CVS commit: src/sys/kern

2019-11-30 Thread Rin Okuyama

On 2019/11/30 23:35, Andrew Doran wrote:

Hmm, it works fine on amd64 and looks OK but me, but I have backed it out
for the time being.


Thanks! Also thank you for working on this area!

rin


Re: CVS commit: src/sys/kern

2019-11-30 Thread Andrew Doran
Hi,

On Sat, Nov 30, 2019 at 04:52:38PM +0900, Rin Okuyama wrote:
> On 2019/11/30 5:50, Andrew Doran wrote:
> > Module Name:src
> > Committed By:   ad
> > Date:   Fri Nov 29 20:50:54 UTC 2019
> > 
> > Modified Files:
> > src/sys/kern: kern_rwlock.c
> > 
> > Log Message:
> > A couple more tweaks to avoid reading the lock word.
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.56 -r1.57 src/sys/kern/kern_rwlock.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> 
> Hi,
> 
> After this commit, GENERIC64 kernel on evbarm-aarch64 does no longer
> boot multiuser anymore with rwlock related panics:
> 
>   panic: kernel diagnostic assertion "vm_map_locked_p(map)" failed: file 
> "../../../../uvm/uvm_map.c", line 1315
> 
> or
> 
>   panic: kernel diagnostic assertion "rw_write_held(>lock)" failed: 
> file "../../../../uvm/uvm_map.c", line 701
> 
> By reverting kern_rwlock.c to rev 1.56, panics disappear as far as
> I can see.

Hmm, it works fine on amd64 and looks OK but me, but I have backed it out
for the time being.

Thank you,
Andrew


Re: CVS commit: src/sys/kern

2019-11-29 Thread Rin Okuyama

On 2019/11/30 5:50, Andrew Doran wrote:

Module Name:src
Committed By:   ad
Date:   Fri Nov 29 20:50:54 UTC 2019

Modified Files:
src/sys/kern: kern_rwlock.c

Log Message:
A couple more tweaks to avoid reading the lock word.


To generate a diff of this commit:
cvs rdiff -u -r1.56 -r1.57 src/sys/kern/kern_rwlock.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Hi,

After this commit, GENERIC64 kernel on evbarm-aarch64 does no longer
boot multiuser anymore with rwlock related panics:

panic: kernel diagnostic assertion "vm_map_locked_p(map)" failed: file 
"../../../../uvm/uvm_map.c", line 1315

or

panic: kernel diagnostic assertion "rw_write_held(>lock)" failed: file 
"../../../../uvm/uvm_map.c", line 701

By reverting kern_rwlock.c to rev 1.56, panics disappear as far as
I can see.

Thanks,
rin

...
Starting nfsd.
[  24.5728628] panic: kernel diagnostic assertion "vm_map_locked_p(map)" failed: file 
"../../../../uvm/uvm_map.c", line 1315
[  24.5880334] cpu0: Begin traceback...
[  24.5880334] trace fp ffc033453aa0
[  24.5954047] fp ffc033453ac0 vpanic() at ffc0004892d0 
netbsd:vpanic+0x160
[  24.6054105] fp ffc033453b30 kern_assert() at ffc0005bc1ac 
netbsd:kern_assert+0x5c
[  24.6154460] fp ffc033453bc0 uvm_map_enter() at ffc000402fb0 
netbsd:uvm_map_enter+0x3b8
[  24.6254813] fp ffc033453c60 uvm_map() at ffc00040351c 
netbsd:uvm_map+0x9c
[  24.6355640] fp ffc033453cf0 uvm_mmap.part.0() at ffc000405ccc 
netbsd:uvm_mmap.part.0+0x13c
[  24.6454881] fp ffc033453d70 sys_mmap() at ffc000406390 
netbsd:sys_mmap+0x1d8
[  24.6570928] fp ffc033453e20 syscall() at ffc787dc 
netbsd:syscall+0x18c
[  24.6683611] tf ffc033453ed0 el0_trap() at ffc76e88 
netbsd:el0_trap
[  24.6794682]  trapframe 0xffc033453ed0 (304 bytes) 
...

...
Starting postfix.
[  30.0175876] panic: kernel diagnostic assertion "rw_write_held(>lock)" failed: 
file "../../../../uvm/uvm_map.c", line 701
[  30.0275865] cpu1: Begin traceback...
[  30.0275865] trace fp ffc034b0edf0
[  30.0375913] fp ffc034b0ee10 vpanic() at ffc0004892d0 
netbsd:vpanic+0x160
[  30.0475892] fp ffc034b0ee80 kern_assert() at ffc0005bc1ac 
netbsd:kern_assert+0x5c
[  30.0575897] fp ffc034b0ef10 vm_map_unlock() at ffc0003ff6c4 
netbsd:vm_map_unlock+0x94
[  30.0675928] fp ffc034b0ef30 uvm_map_enter() at ffc000402f04 
netbsd:uvm_map_enter+0x30c
[  30.0775928] fp ffc034b0efd0 uvm_map() at ffc00040351c 
netbsd:uvm_map+0x9c
[  30.0775928] fp ffc034b0f060 uvm_pagermapin() at ffc00040b69c 
netbsd:uvm_pagermapin+0xac
[  30.0975934] fp ffc034b0f0e0 genfs_getpages() at ffc0004f5ff8 
netbsd:genfs_getpages+0xbb8
[  30.1075948] fp ffc034b0f270 VOP_GETPAGES() at ffc0004f38b4 
netbsd:VOP_GETPAGES+0x44
[  30.1175956] fp ffc034b0f2e0 ubc_fault() at ffc0003f6f30 
netbsd:ubc_fault+0x168
[  30.1275959] fp ffc034b0f370 uvm_fault_internal() at ffc0003f9a18 
netbsd:uvm_fault_internal+0x478
[  30.1375967] fp ffc034b0f580 data_abort_handler() at ffc79f88 
netbsd:data_abort_handler+0x100
[  30.1475968] tf ffc034b0f600 el1_trap() at ffc76e24 
netbsd:el1_trap
[  30.1575982]  trapframe 0xffc034b0f600 (304 bytes) 
...


Re: CVS commit: src/sys/kern

2019-11-07 Thread Christos Zoulas
On Nov 7,  6:08pm, n...@gmx.com (Kamil Rytarowski) wrote:
-- Subject: Re: CVS commit: src/sys/kern

| Please review:
| 
| http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
| 
| This patch works for me.
| 
| Patch inspired by:
| 
| Avoid misaligned access in disklabel(8) in find_label()
| https://github.com/NetBSD/src/commit/19bd3d170c8fd67052ca4ca20151cd77d893ab=
| 88
| 
| After landing it I will revert "Avoid unaligned pointer arithmetic in
| check_label_magic()".

Yeah, let's do that.

christos


Re: CVS commit: src/sys/kern

2019-11-07 Thread Valery Ushakov
On Thu, Nov 07, 2019 at 19:06:29 +0100, Martin Husemann wrote:

> OK, why is it 8 byte aligned? Checking
> 
> > revision 1.108
> > date: 2011-01-18 20:52:24 +0100;  author: matt;  state: Exp;  lines: +2 -1;
> > Make struct disklabel 8 byte aligned.  This increases its size by 4 bytes
> > on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal
> > with the older/smaller diskabel size.  This change makes disklabel the
> > same for both IPL32 and LP64 platforms.
> 
> Argh! Somehow I dimly remember having been here already ...
> I think I proposed to change the loop to 8 byte increments back then, but
> noone knows what odd systems it will the stop working on (none at all is my
> personal bet).

I gather the alignment mess is caused by pointers d_un.un_b.un_d_boot0
and d_un.un_b.un_d_boot1 that are inside the d_un union and are NOT
part of the actual disklabel: "These are returned when using
getdiskbyname(3) to retrieve the values from /etc/disktab."

So that was completely self-inflicted.  The loop is fine for the
on-disk structure.

Now the question is how to dig our way out of this, b/c unfortunately
it's a public interface.  But the mindless memcpy should be the last
resort IMO.

-uwe


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 19:32, Valery Ushakov wrote:
> On Thu, Nov 07, 2019 at 18:08:40 +0100, Kamil Rytarowski wrote:
> 
>> On 07.11.2019 16:45, Kamil Rytarowski wrote:
>>> On 07.11.2019 16:26, Martin Husemann wrote:
 On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
> On 07.11.2019 14:25, Valery Ushakov wrote:
>> If the sanitizer does complain about other uses, there is little point
>> in fixing one instance and not the others.
>
> We already agreed with Christos that this is appeasing of GCC. If you
> want to scan the whole kernel (or whole C) file for more occurrences of
> violations - please go for it.

 No. The commit needs to be reverted, and then

  a) either the root cause for the unaligned address be fixed or
  b) some other means be found to make the sanitizer shut up

 As uwe said: papering over a tiny detail that *never* hits in the real
 world but potentialy hiding a real issue is not the way to go.

>>>
>>> I don't have a readily available reproducer locally but it was breaking
>>> syzbot from booting after the switch to gcc8. I will fix it differently
>>> aligning the whole struct (so the same approach as we use in userland)
>>> and backout this change.
>>>
>>
>> Please review:
>>
>> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
>>
>> This patch works for me.
> 
> What happens if you change check_label_magic() to use direct member
> accesses (as the code did before xtos change it) instead of memcmp?
> Does that shup up the sanitizer?  I assume it should as it doesn't
> complain about other member accesses.  I'd strongly prefer this change
> for now.
> 
> -uwe
> 

I have got no opinion on this. It will work now. If you prefer it,
please go for it.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Valery Ushakov
On Thu, Nov 07, 2019 at 18:08:40 +0100, Kamil Rytarowski wrote:

> On 07.11.2019 16:45, Kamil Rytarowski wrote:
> > On 07.11.2019 16:26, Martin Husemann wrote:
> >> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
> >>> On 07.11.2019 14:25, Valery Ushakov wrote:
>  If the sanitizer does complain about other uses, there is little point
>  in fixing one instance and not the others.
> >>>
> >>> We already agreed with Christos that this is appeasing of GCC. If you
> >>> want to scan the whole kernel (or whole C) file for more occurrences of
> >>> violations - please go for it.
> >>
> >> No. The commit needs to be reverted, and then
> >>
> >>  a) either the root cause for the unaligned address be fixed or
> >>  b) some other means be found to make the sanitizer shut up
> >>
> >> As uwe said: papering over a tiny detail that *never* hits in the real
> >> world but potentialy hiding a real issue is not the way to go.
> >>
> > 
> > I don't have a readily available reproducer locally but it was breaking
> > syzbot from booting after the switch to gcc8. I will fix it differently
> > aligning the whole struct (so the same approach as we use in userland)
> > and backout this change.
> > 
> 
> Please review:
> 
> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
> 
> This patch works for me.

What happens if you change check_label_magic() to use direct member
accesses (as the code did before xtos change it) instead of memcmp?
Does that shup up the sanitizer?  I assume it should as it doesn't
complain about other member accesses.  I'd strongly prefer this change
for now.

-uwe


Re: CVS commit: src/sys/kern

2019-11-07 Thread Martin Husemann
On Thu, Nov 07, 2019 at 06:46:48PM +0100, Kamil Rytarowski wrote:
> member access within misaligned address 0x942d3de8c03c for type
> 'const struct disklabel' which requires 8 byte alignment

OK, why is it 8 byte aligned? Checking

> revision 1.108
> date: 2011-01-18 20:52:24 +0100;  author: matt;  state: Exp;  lines: +2 -1;
> Make struct disklabel 8 byte aligned.  This increases its size by 4 bytes
> on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal
> with the older/smaller diskabel size.  This change makes disklabel the
> same for both IPL32 and LP64 platforms.

Argh! Somehow I dimly remember having been here already ...
I think I proposed to change the loop to 8 byte increments back then, but
noone knows what odd systems it will the stop working on (none at all is my
personal bet).

So I guess your patch is the best way forward.

Martin


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 18:20, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 06:08:40PM +0100, Kamil Rytarowski wrote:
>> Please review:
>>
>> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
>>
>> This patch works for me.
> 
> Yes, I believe that it does - but why is it needed?
> 

syzbot reported on boot:

member access within misaligned address 0x942d3de8c03c for type
'const struct disklabel' which requires 8 byte alignment

> dlp = (void *)a->bp->b_data;
> 
> Here we can assume that b_data is properly aligned (likely even 512 byte
> aligned or similar).
> 

I just have got this backtrace:

[   1.3496393] panic: UBSan: Undefined Behavior in
/syzkaller/managers/netbsd-kubsan/kernel/sys/kern/subr_disk_mbr.c:574:9,
member access within misaligned address 0x942d3de8c03c for type
'const struct disklabel' which requires 8 byte alignment

[   1.3663828] cpu0: Begin traceback...
[   1.3689943] vpanic() at netbsd:vpanic+0x2aa sys/kern/subr_prf.c:336
[   1.3788101] isAlreadyReported() at netbsd:isAlreadyReported
[   1.3988418] HandleTypeMismatch.part.1() at
netbsd:HandleTypeMismatch.part.1+0xcc
[   1.4088591] HandleTypeMismatch() at netbsd:HandleTypeMismatch+0x7b
sys/../common/lib/libc/misc/ubsan.c:408
[   1.4288896] check_label_magic() at netbsd:check_label_magic+0x9f
sys/kern/subr_disk_mbr.c:574
[   1.4389098] validate_label() at netbsd:validate_label+0x1b4
sys/kern/subr_disk_mbr.c:626
[   1.4489214] look_netbsd_part() at netbsd:look_netbsd_part+0x3ce
sys/kern/subr_disk_mbr.c:526
[   1.4689530] scan_mbr() at netbsd:scan_mbr+0x24a
sys/kern/subr_disk_mbr.c:234
[   1.4789689] readdisklabel() at netbsd:readdisklabel+0x412
sys/kern/subr_disk_mbr.c:448
[   1.4898443] dk_getdisklabel() at netbsd:dk_getdisklabel+0x192
sys/dev/dksubr.c:931
[   1.5090177] dk_open() at netbsd:dk_open+0x456 sys/dev/dksubr.c:177
[   1.5190333] sdopen() at netbsd:sdopen+0x114 sys/dev/scsipi/sd.c:543
[   1.5290486] cdev_open() at netbsd:cdev_open+0xe5
sys/kern/subr_devsw.c:871
[   1.5490803] spec_open() at netbsd:spec_open+0x3ad
sys/miscfs/specfs/spec_vnops.c:562
[   1.5591508] VOP_OPEN() at netbsd:VOP_OPEN+0x113 sys/kern/vnode_if.c:298
[   1.5791291] dkwedge_discover() at netbsd:dkwedge_discover+0xcf
sys/dev/dkwedge/dk.c:931
[   1.5891449] sdattach() at netbsd:sdattach+0x53f sys/dev/scsipi/sd.c:362
[   1.5991611] config_attach_loc() at netbsd:config_attach_loc+0x432
sys/kern/subr_autoconf.c:1604
[   1.6191925] scsi_probe_bus() at netbsd:scsi_probe_bus+0xad8
scsi_probe_device sys/dev/scsipi/scsiconf.c:1035 [inline]
[   1.6191925] scsi_probe_bus() at netbsd:scsi_probe_bus+0xad8
sys/dev/scsipi/scsiconf.c:413
[   1.6292092] scsibus_discover_thread() at
netbsd:scsibus_discover_thread+0xdd scsibus_config
sys/dev/scsipi/scsiconf.c:320 [inline]
[   1.6292092] scsibus_discover_thread() at
netbsd:scsibus_discover_thread+0xdd sys/dev/scsipi/scsiconf.c:285
[   1.6392253] cpu0: End traceback...
[   1.6392253] fatal breakpoint trap in supervisor mode
[   1.6392253] trap type 1 code 0 rip 0x8021 cs 0x8 rflags
0x282 cr2 0 ilevel 0 rsp 0xbc80a5c02280
[   1.6555812] curlwp 0x942c2bcce9c0 pid 0.28 lowest kstack
0xbc80a5bff2c0

https://syzkaller.appspot.com/bug?extid=56769dece0ec3e35731e

>   for (;; dlp = (void *)((char *)dlp + sizeof(uint32_t))) {
> 
> ugly as it is written, this still should ensure proper (4 byte) alignement
> of all later values of dlp.

We actually need 8-byte alignment to make a compiler appeased.. so this
patch is still good (if I remember correctly it was the original
motivation for patching the userland part).

Can I land it?

> 
> Do we have any architectures with odd LABELOFFSET?
> I only found 0, 64, 128 and 516, none of those should cause alignment
> issues in the code following.
> 
> So where is the problem?
> 

Could that be a half-corrupted image after some crash? The previous
patch actually fixed booting.. so I don't know.

> Martin
> 




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Martin Husemann
On Thu, Nov 07, 2019 at 06:08:40PM +0100, Kamil Rytarowski wrote:
> Please review:
> 
> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
> 
> This patch works for me.

Yes, I believe that it does - but why is it needed?

dlp = (void *)a->bp->b_data;

Here we can assume that b_data is properly aligned (likely even 512 byte
aligned or similar).

for (;; dlp = (void *)((char *)dlp + sizeof(uint32_t))) {

ugly as it is written, this still should ensure proper (4 byte) alignement
of all later values of dlp.

Do we have any architectures with odd LABELOFFSET?
I only found 0, 64, 128 and 516, none of those should cause alignment
issues in the code following.

So where is the problem?

Martin


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 16:45, Kamil Rytarowski wrote:
> On 07.11.2019 16:26, Martin Husemann wrote:
>> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
>>> On 07.11.2019 14:25, Valery Ushakov wrote:
 If the sanitizer does complain about other uses, there is little point
 in fixing one instance and not the others.
>>>
>>> We already agreed with Christos that this is appeasing of GCC. If you
>>> want to scan the whole kernel (or whole C) file for more occurrences of
>>> violations - please go for it.
>>
>> No. The commit needs to be reverted, and then
>>
>>  a) either the root cause for the unaligned address be fixed or
>>  b) some other means be found to make the sanitizer shut up
>>
>> As uwe said: papering over a tiny detail that *never* hits in the real
>> world but potentialy hiding a real issue is not the way to go.
>>
> 
> I don't have a readily available reproducer locally but it was breaking
> syzbot from booting after the switch to gcc8. I will fix it differently
> aligning the whole struct (so the same approach as we use in userland)
> and backout this change.
> 

Please review:

http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt

This patch works for me.

Patch inspired by:

Avoid misaligned access in disklabel(8) in find_label()
https://github.com/NetBSD/src/commit/19bd3d170c8fd67052ca4ca20151cd77d893ab88

After landing it I will revert "Avoid unaligned pointer arithmetic in
check_label_magic()".



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Steffen Nurpmeso
David Young wrote in <20191107155806.gl1...@pobox.com>:
 |On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote:
 |> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
 |>> On 07.11.2019 14:25, Valery Ushakov wrote:
 ..
 |I think the problem is that if you have the series of statements,
 |
 |element_t *e = >element;
 |
 |if (s == NULL)
 |return;
 ...
 |There is probably an argument to be made that in a
 |segmented/tagged/capability architecture that has run C programs
 |(8086; Burroughs Large Systems) or may run them in the future (CHERI),
 |&(NULL)->element cannot sensibly be computed.

You mean they will render any non-compiler-builtin approach to
create field_offsetof() and field_sizeof() impossible.

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 17:20, Kamil Rytarowski wrote:
> On 07.11.2019 17:08, Martin Husemann wrote:
>> On Thu, Nov 07, 2019 at 04:56:16PM +0100, Kamil Rytarowski wrote:
>>> 6.3.2.1 C11
>>>
>>> 'An lvalue is an expression (with an object type other than void) that
>>> potentially designates an object'
>>>
>>> This means that real dereference is not needed, only a potential. And
>>> there are special cases of pointer arithmetic.
>>
>> But 6.5.3.2 says that taking the address of a pointer expression does
>> not "evaluate the * oparator", but instead is shortcut to address addition.
>>
> 
> I think that this does not apply as & is applied already to lvalue. This
> part of standard specifies &*p, not something like &((*p).x).
> 

To be clear the third case (next to applying to * and lvalue) is
applying & to the result of []. Unfortunately p->x goes into the lvalue
section.

This is how I interpret it (and find it as unreasonable in practice).

>> The annotations talk about various special cases and not the concreate one
>> at hand, but in my reading the intention is clear and this part should be
>> clarified.
>>
> 
> I can agree that this would be nicer to be defined, unfortunately I
> think that we can merely ask for exception to apply & to -> operator.
> 
>> Martin
>>
> 
> 




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Valery Ushakov
On Thu, Nov 07, 2019 at 09:58:06 -0600, David Young wrote:

> On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote:
> > On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
> > > On 07.11.2019 14:25, Valery Ushakov wrote:
> > > > If the sanitizer does complain about other uses, there is little point
> > > > in fixing one instance and not the others.
> > > 
> > > We already agreed with Christos that this is appeasing of GCC. If you
> > > want to scan the whole kernel (or whole C) file for more occurrences of
> > > violations - please go for it.
> > 
> > No. The commit needs to be reverted, and then
> > 
> >  a) either the root cause for the unaligned address be fixed or
> >  b) some other means be found to make the sanitizer shut up
> > 
> > As uwe said: papering over a tiny detail that *never* hits in the real
> > world but potentialy hiding a real issue is not the way to go.
> > 
> > Martin
> > 
> > P.S.: Independend of this I would still like an official C standard
> > clarification; in my reading a simple address calculation is not
> > accessing an object through a pointer (which would be the undefined
> > behaviour). If the C standard is not clear on this, it needs to be
> > improved.
> 
> I think the problem is that if you have the series of statements,
> 
> element_t *e = >element;
> 
> if (s == NULL)
> return;
> 
> then the comparison with NULL implies that in this scope, s could
> be NULL.  NULL does not necessarily have any "arithmetic" relationship
> to any other pointer---by that rationale, you probably cannot assign
> any alignment to it---so there is no sensible value that you can
> give to e.

This is not what the changed code does.  The code in question has

  struct disklabel *dlp = ...;

apparently gcc complains about

  memcmp(>d_magic, ...)

but later the code uses e.g. dlp->d_partitions (right after the
check_label_magic call) and other memebers.  So it's very suspicious
that one usage is flagged and others are not.

Until very recently the magic check was also explicitly comparing
dlp->d_magic != DISKMAGIC, etc.  So may be we should stop pretending
and rewrite check_label_magic() to use that instead of memcmp.  (And
then fix all dlp->foo in one swoop).

If my I interpretation is wrong, I would be glad to be corrected.


> There is probably an argument to be made that in a
> segmented/tagged/capability architecture that has run C programs
> (8086; Burroughs Large Systems) or may run them in the future (CHERI),
> &(NULL)->element cannot sensibly be computed.

Amen :).  I actually did encounter problems like that when compiling
software on Xenix 286 ages ago (e.g. 0 instead of NULL passed as the
last argument to exec).  While that is a fascinating excercise, it's
not what happens here.


-uwe


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 17:09, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 09:58:06AM -0600, David Young wrote:
>> I think the problem is that if you have the series of statements,
>>
>> element_t *e = >element;
>>
>> if (s == NULL)
>> return;
> 
> Note that this example has *nothing* in common with Kamil's code change.
> He cited it as an example of the sanitize being usefull, but it only
> distracted from the real issue.
> 
> Martin
> 

This is the very similar case, except UB is a different part, in
misaligned pointer rather that dereferened NULL.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 17:08, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 04:56:16PM +0100, Kamil Rytarowski wrote:
>> 6.3.2.1 C11
>>
>> 'An lvalue is an expression (with an object type other than void) that
>> potentially designates an object'
>>
>> This means that real dereference is not needed, only a potential. And
>> there are special cases of pointer arithmetic.
> 
> But 6.5.3.2 says that taking the address of a pointer expression does
> not "evaluate the * oparator", but instead is shortcut to address addition.
> 

I think that this does not apply as & is applied already to lvalue. This
part of standard specifies &*p, not something like &((*p).x).

> The annotations talk about various special cases and not the concreate one
> at hand, but in my reading the intention is clear and this part should be
> clarified.
> 

I can agree that this would be nicer to be defined, unfortunately I
think that we can merely ask for exception to apply & to -> operator.

> Martin
> 




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Martin Husemann
On Thu, Nov 07, 2019 at 09:58:06AM -0600, David Young wrote:
> I think the problem is that if you have the series of statements,
> 
> element_t *e = >element;
> 
> if (s == NULL)
> return;

Note that this example has *nothing* in common with Kamil's code change.
He cited it as an example of the sanitize being usefull, but it only
distracted from the real issue.

Martin


Re: CVS commit: src/sys/kern

2019-11-07 Thread Martin Husemann
On Thu, Nov 07, 2019 at 04:56:16PM +0100, Kamil Rytarowski wrote:
> 6.3.2.1 C11
> 
> 'An lvalue is an expression (with an object type other than void) that
> potentially designates an object'
> 
> This means that real dereference is not needed, only a potential. And
> there are special cases of pointer arithmetic.

But 6.5.3.2 says that taking the address of a pointer expression does
not "evaluate the * oparator", but instead is shortcut to address addition.

The annotations talk about various special cases and not the concreate one
at hand, but in my reading the intention is clear and this part should be
clarified.

Martin


Re: CVS commit: src/sys/kern

2019-11-07 Thread David Young
On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
> > On 07.11.2019 14:25, Valery Ushakov wrote:
> > > If the sanitizer does complain about other uses, there is little point
> > > in fixing one instance and not the others.
> > 
> > We already agreed with Christos that this is appeasing of GCC. If you
> > want to scan the whole kernel (or whole C) file for more occurrences of
> > violations - please go for it.
> 
> No. The commit needs to be reverted, and then
> 
>  a) either the root cause for the unaligned address be fixed or
>  b) some other means be found to make the sanitizer shut up
> 
> As uwe said: papering over a tiny detail that *never* hits in the real
> world but potentialy hiding a real issue is not the way to go.
> 
> Martin
> 
> P.S.: Independend of this I would still like an official C standard
> clarification; in my reading a simple address calculation is not
> accessing an object through a pointer (which would be the undefined
> behaviour). If the C standard is not clear on this, it needs to be
> improved.

I think the problem is that if you have the series of statements,

element_t *e = >element;

if (s == NULL)
return;

then the comparison with NULL implies that in this scope, s could
be NULL.  NULL does not necessarily have any "arithmetic" relationship
to any other pointer---by that rationale, you probably cannot assign
any alignment to it---so there is no sensible value that you can
give to e.

There is probably an argument to be made that in a
segmented/tagged/capability architecture that has run C programs
(8086; Burroughs Large Systems) or may run them in the future (CHERI),
&(NULL)->element cannot sensibly be computed.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 16:49, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 04:45:31PM +0100, Kamil Rytarowski wrote:
>> Unfortunately the C committee went into the opposite direction here and
>> specified a potential dereference.
> 
> Where?
> 
> Martin
> 

6.3.2.1 C99

"An lvalue is an expression with an object type or an incomplete type
other than void"

6.3.2.1 C11

'An lvalue is an expression (with an object type other than void) that
potentially designates an object'

This means that real dereference is not needed, only a potential. And
there are special cases of pointer arithmetic.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Martin Husemann
On Thu, Nov 07, 2019 at 04:45:31PM +0100, Kamil Rytarowski wrote:
> Unfortunately the C committee went into the opposite direction here and
> specified a potential dereference.

Where?

Martin


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 16:26, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
>> On 07.11.2019 14:25, Valery Ushakov wrote:
>>> If the sanitizer does complain about other uses, there is little point
>>> in fixing one instance and not the others.
>>
>> We already agreed with Christos that this is appeasing of GCC. If you
>> want to scan the whole kernel (or whole C) file for more occurrences of
>> violations - please go for it.
> 
> No. The commit needs to be reverted, and then
> 
>  a) either the root cause for the unaligned address be fixed or
>  b) some other means be found to make the sanitizer shut up
> 
> As uwe said: papering over a tiny detail that *never* hits in the real
> world but potentialy hiding a real issue is not the way to go.
> 

I don't have a readily available reproducer locally but it was breaking
syzbot from booting after the switch to gcc8. I will fix it differently
aligning the whole struct (so the same approach as we use in userland)
and backout this change.

> Martin
> 
> P.S.: Independend of this I would still like an official C standard
> clarification; in my reading a simple address calculation is not
> accessing an object through a pointer (which would be the undefined
> behaviour). If the C standard is not clear on this, it needs to be
> improved.
> 

Unfortunately the C committee went into the opposite direction here and
specified a potential dereference. All I can do now is to add another
exception, [x] is allowed and it is not far from >x. It is also a
tricky part as some things are unequally documented for p->x and for
(*p).x... so not sure if it is worth trying out really... especially
that offsetof() as defined for this purpose.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Martin Husemann
On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
> On 07.11.2019 14:25, Valery Ushakov wrote:
> > If the sanitizer does complain about other uses, there is little point
> > in fixing one instance and not the others.
> 
> We already agreed with Christos that this is appeasing of GCC. If you
> want to scan the whole kernel (or whole C) file for more occurrences of
> violations - please go for it.

No. The commit needs to be reverted, and then

 a) either the root cause for the unaligned address be fixed or
 b) some other means be found to make the sanitizer shut up

As uwe said: papering over a tiny detail that *never* hits in the real
world but potentialy hiding a real issue is not the way to go.

Martin

P.S.: Independend of this I would still like an official C standard
clarification; in my reading a simple address calculation is not
accessing an object through a pointer (which would be the undefined
behaviour). If the C standard is not clear on this, it needs to be
improved.


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 14:25, Valery Ushakov wrote:
> If the sanitizer does complain about other uses, there is little point
> in fixing one instance and not the others.

We already agreed with Christos that this is appeasing of GCC. If you
want to scan the whole kernel (or whole C) file for more occurrences of
violations - please go for it.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/kern

2019-11-07 Thread Valery Ushakov
On Thu, Nov 07, 2019 at 15:48:55 +0300, Valery Ushakov wrote:

> On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote:
> 
> > On 07.11.2019 13:17, Valery Ushakov wrote:
> > > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote:
> > > 
> > >> I have checked received the following patch and received a feedback from
> > >> a LLVM developer.
> > >>
> > >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote:
> > >>> I've consulted with some people and _presumably_ (to the degree one
> > >>> can be sure about bitter corner cases of C/C++ :)) this is a correct
> > >>> fix (and formally correct warnings from ubsan).
> > >>> As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as
> > >>> special case of not being a dereference, but rather effectively an
> > >>> address calculation. But >m is not. Thus it is interpreted as a
> > >>> dereference that produces an lvalue and then taking address of that
> > >>> lvalue. At the point of dereference we have UB. Your fix avoids the
> > >>> dereference.
> > > 
> > > The context is lost in the thread, but the original change was about
> > > >d_magic as far as I can figure out.  If the claim is that that's
> > > UB b/c dlp is improperly aligned, then why the half of the rest of the
> > > file is not UB as it uses the same "dlp" pointer to access other
> > > members of the disklabel.
> > 
> > We were already addressing various reports for disklabel related code in
> > the kernel and userland. In userland we as far as I recall just copy the
> > struct into an aligned promptly pointer.
> > 
> > There might be more problems, but we address them as they pop up.
> 
> That seems counterintuitive.  There's the root cause and when/if that
> root cause is fixed, then this particular problem will be fixed as
> well.  The concern obviously is that when the root problem is fixed,
> this change will be forgotten and the unnecessarily uglified code will
> be just left over as it currently is.
> 
> So the change is extremely questionable from that point of view.  It
> creates the illusion of things being "fixed" while in the longer run
> I'd say it harms the code.  And as I said, if the sanitizer flags that
> place as UB and doesn't flag dozens of dlp->foo accesses elsewhere in
> that file, then either I don't understand what the sanitizer complains
> about, or it's not a very good sanitizer and we shouldn't care to shut
> it up in this random place that triggers it.

Lest the legalese part of this sub-thread distract from this point let
me re-iterate.  I'm not questioning primarily whether that
>d_magic is UB or not (it most likely is).  What irks me about
this change is that we tweak one random instance of a larger problem
and disregard the rest of it.

If I misunderstand the problem and other uses of dpl->foo in the same
file are ok, please, kindly point this out to me.

If the other uses are indeed problematic, then does or doesn't the
sanitizer complain about them, like it complains about that check that
was "fixed" in the original commit?

If the sanitizer does complain about other uses, there is little point
in fixing one instance and not the others.

If the sanitizer does NOT complain about other uses, then please find
a different way to shup the stupid thing up.  (Which is also how I
read the responses from Martin and Christos, but that's just my
interpretation).

-uwe


Re: CVS commit: src/sys/kern

2019-11-07 Thread Valery Ushakov
On Thu, Nov 07, 2019 at 13:59:37 +0100, Kamil Rytarowski wrote:

> On 07.11.2019 13:48, Valery Ushakov wrote:
> > On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote:
> > 
> >> On 07.11.2019 13:17, Valery Ushakov wrote:
> >>> On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote:
> >>> As a side note - the C99 standard contains "derefer" exactly once, in
> >>> a footnote.  Since we have ended up in the darkest corners of
> >>> legalistic exegesis, please, can we avoid using the word that is,
> >>> technically speaking, meaningless as far as this discussion is
> >>> concerned?
> >>
> >> Unary * oprator. C++ specified term "dereferenceable" in the context of
> >> the unary * operator.
> > 
> > This is C code and the C standard is hard enough as it is already.
> > Please, can we put the C++ aside for a moment?
> 
> No. The kernel was already patched (years ago) to build as a valid C++
> software.

"No" what?  This is C code.  If it also happens to be a valid C++
code, good for it, but that is a separate matter.  There's a claim
made about that code that it triggers UB according to the C standard.
That claim can be meaningfully dicussed in that legal(ese) framework
only.

Only after the meaning of the competing claims about that code is
clarified within its "native" framework can we consider C++
interpretation of the same code as a followup and whatever interplay
there is between the standards.

So, if that code is supposed to be valid C code *and* valid C++ code,
then it should be at least valid C code and so, please, can we for now
stick to that part of the "and"?

-uwe


Re: CVS commit: src/sys/kern

2019-11-07 Thread Kamil Rytarowski
On 07.11.2019 13:48, Valery Ushakov wrote:
> On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote:
> 
>> On 07.11.2019 13:17, Valery Ushakov wrote:
>>> On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote:
>>> As a side note - the C99 standard contains "derefer" exactly once, in
>>> a footnote.  Since we have ended up in the darkest corners of
>>> legalistic exegesis, please, can we avoid using the word that is,
>>> technically speaking, meaningless as far as this discussion is
>>> concerned?
>>
>> Unary * oprator. C++ specified term "dereferenceable" in the context of
>> the unary * operator.
> 
> This is C code and the C standard is hard enough as it is already.
> Please, can we put the C++ aside for a moment?
> 

No. The kernel was already patched (years ago) to build as a valid C++
software.

> 
> -uwe
> 




signature.asc
Description: OpenPGP digital signature


  1   2   3   4   5   6   7   >