kern/52643: npfctl reload on rump kernels fails randomly

[ozaki-r%netbsd.org@localhost]

>Number:         52643
>Category:       kern
>Synopsis:       npfctl reload on rump kernels fails randomly
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Oct 23 07:45:00 +0000 2017
>Originator:     Ryota Ozaki
>Release:        -current (and maybe earlier releases)
>Organization:
>Environment:
NetBSD rangeley 8.99.3 NetBSD 8.99.3 (RANGELEY) #77: Wed Oct  4 10:23:38 JST 2017  ozaki-r@rangeley:(hidden) amd64
>Description:
npfctl reload fails sometimes on rump kernels with EIO like this:

  tc-so:Executing command [ env LD_PRELOAD=/usr/lib/librumphijack.so RUMPHIJACK=path=/rump,socket=all:nolocal,sysctl=yes,blanket=/dev/npf npfctl reload ./npf.conf ]
  tc-se:Fail: incorrect exit status: 1, expected: 0
  tc-se:stdout:
  tc-se:
  tc-se:stderr:
  tc-se:npfctl: source  line 333501697
  tc-se:npfctl: object: 535
  tc-se:npfctl: npfctl_config_send: Input/output error
  tc-se:

The error message is generated by libnpf that uses libprop.

npfctl uses npf_config_submit of libnpf to call ioctl to the kernel.
libnpf uses prop_dictionary_sendrecv_ioctl to call ioctl and
get an errno of the ioctl by prop_dictionary_get_int32.
On npfctl reload on rump kernels, prop_dictionary_sendrecv_ioctl succeeds
but prop_dictionary_get_int32 fails because errno isn't set correctly.

It seems that npfctl_load that is a kernel function of npf and called
via the prop_dictionary_sendrecv_ioctl doesn't set the errno to the return
value of libprop: https://nxr.netbsd.org/xref/src/sys/net/npf/npf_ctl.c#635

          /* Error report. */
  #if !defined(_NPF_TESTING) && !defined(_NPF_STANDALONE)
          prop_dictionary_set_int32(errdict, "errno", error);
          prop_dictionary_copyout_ioctl(pref, cmd, errdict);
          prop_object_release(errdict);
          error = 0;
  #endif
          return error;

The code setting an errno is trimmed on rump kernels since by default
rump kernels set _NPF_TESTING. If we enable the code, npfctl reload
works expectedly.

Note that npfctl reload sometimes succeeds, I guess, because npf_config_submit
tries to read an errno from a uninitialized memory and it sometimes works luckily.
>How-To-Repeat:
# From an ATF test script that is under development

rump_server -lrumpnet -lrumpnet_net -lrumpnet_netinet -lrumpnet_shmif -lrumpdev -lrumpvfs -lrumpdev_bpf -lrumpnet_bpfjit -lrumpnet_npf -lrumpnet_netinet6 unix://npf_nat_local

export RUMP_SERVER=unix://npf_nat_local
rump.ifconfig shmif0 create
rump.ifconfig shmif0 linkstr ./bus_npf_local
rump.ifconfig shmif1 create
rump.ifconfig shmif1 linkstr ./bus_npf_nat

rump.sysctl -q -w net.inet.ip.dad_count=0
rump.ifconfig shmif0 10.0.1.1/24
rump.ifconfig shmif1 20.0.0.1/24
rump.sysctl -q -w net.inet.ip.forwarding=1
rump.route -n add -net 10.0.2.0 20.0.0.2

cat > ./npf.conf <<-EOF
set bpf.jit off
\$int_if = inet4(shmif0)
\$ext_if = inet4(shmif1)
\$localnet = { 10.0.1.0/24 }
map \$ext_if dynamic \$localnet -> \$ext_if
group "external" on \$ext_if {
        pass stateful out final all
}
group "internal" on \$int_if {
        block in all
        pass in final from \$localnet
        pass out final all
}
group default {
        pass final on lo0 all
        block all
}
EOF

env LD_PRELOAD=/usr/lib/librumphijack.so RUMPHIJACK=path=/rump,socket=all:nolocal,sysctl=yes,blanket=/dev/npf npfctl reload ./npf.conf
>Fix:
As described above, enabling the code fixes the issue, but I'm not sure
if the fix is appropriate.

diff --git a/sys/net/npf/npf_ctl.c b/sys/net/npf/npf_ctl.c
index b1af2d347ac..43105db9c85 100644
--- a/sys/net/npf/npf_ctl.c
+++ b/sys/net/npf/npf_ctl.c
@@ -633,12 +633,12 @@ fail:
        prop_object_release(npf_dict);
 
        /* Error report. */
-#if !defined(_NPF_TESTING) && !defined(_NPF_STANDALONE)
+//#if !defined(_NPF_TESTING) && !defined(_NPF_STANDALONE)
        prop_dictionary_set_int32(errdict, "errno", error);
        prop_dictionary_copyout_ioctl(pref, cmd, errdict);
        prop_object_release(errdict);
        error = 0;
-#endif
+//#endif
        return error;
 }