NetBSD Problem Report #53666
From www@NetBSD.org Thu Oct 11 14:08:54 2018
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 67C5F7A1B5
for <gnats-bugs@gnats.NetBSD.org>; Thu, 11 Oct 2018 14:08:54 +0000 (UTC)
Message-Id: <20181011140853.5ADD67A26E@mollari.NetBSD.org>
Date: Thu, 11 Oct 2018 14:08:53 +0000 (UTC)
From: rokuyama@rk.phys.keio.ac.jp
Reply-To: rokuyama@rk.phys.keio.ac.jp
To: gnats-bugs@NetBSD.org
Subject: tcpdump for i386 does not work with COMPAT_NETBSD32 on amd64
X-Send-Pr-Version: www-1.0
>Number: 53666
>Category: kern
>Synopsis: tcpdump for i386 does not work with COMPAT_NETBSD32 on amd64
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: kern-bug-people
>State: analyzed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Thu Oct 11 14:10:00 +0000 2018
>Closed-Date:
>Last-Modified: Mon Jul 22 16:30:01 +0000 2019
>Originator: Rin Okuyama
>Release: 8.99.25
>Organization:
School of Science and Technology, Meiji University
>Environment:
NetBSD latipes 8.99.25 NetBSD 8.99.25 (AMD64) #21: Thu Sep 13 19:27:53 JST 2018 rin@latipes:/var/build/src/sys/arch/amd64/compile/AMD64 amd64
>Description:
tcpdump compiled for i386 does not work with COMPAT_NETBSD32 on amd64.
>How-To-Repeat:
# uname -p
x86_64
# /somewhere/netbsd32/usr/bin/tcpdump
tcpdump: BIOCSRTIMEOUT: Invalid argument
#
>Fix:
This is because argument for BIOCSRTIMEOUT ioctl for bpf(4),
struct timeval *, is not appropriately converted. Note that this
problem does not affect for architectures that have 8-byte
alignment for int64 on 32-bit environment, i.e., struct timeval
and netbsd32_timeval coincide.
This patch fixes the problem:
http://www.netbsd.org/~rin/netbsd32_ioctl_20181011.patch
----
Index: sys/compat/netbsd32/netbsd32_ioctl.c
===================================================================
RCS file: /home/netbsd/src/sys/compat/netbsd32/netbsd32_ioctl.c,v
retrieving revision 1.95
diff -p -u -r1.95 netbsd32_ioctl.c
--- sys/compat/netbsd32/netbsd32_ioctl.c 24 Sep 2018 21:15:39 -0000 1.95
+++ sys/compat/netbsd32/netbsd32_ioctl.c 11 Oct 2018 12:16:35 -0000
@@ -1388,6 +1388,12 @@ netbsd32_ioctl(struct lwp *l, const stru
IOCTL_STRUCT_CONV_TO(BIOCSUDPF, bpf_program);
case BIOCGDLTLIST32:
IOCTL_STRUCT_CONV_TO(BIOCGDLTLIST, bpf_dltlist);
+ case BIOCSRTIMEOUT32:
+#define netbsd32_to_timeval(p, s32p, cmd) netbsd32_to_timeval(p, s32p)
+#define netbsd32_from_timeval(p, s32p, cmd) netbsd32_from_timeval(p, s32p)
+ IOCTL_STRUCT_CONV_TO(BIOCSRTIMEOUT, timeval);
+#undef netbsd32_to_timeval
+#undef netbsd32_from_timeval
case WSDISPLAYIO_ADDSCREEN32:
IOCTL_STRUCT_CONV_TO(WSDISPLAYIO_ADDSCREEN, wsdisplay_addscreendata);
Index: sys/compat/netbsd32/netbsd32_ioctl.h
===================================================================
RCS file: /home/netbsd/src/sys/compat/netbsd32/netbsd32_ioctl.h,v
retrieving revision 1.63
diff -p -u -r1.63 netbsd32_ioctl.h
--- sys/compat/netbsd32/netbsd32_ioctl.h 24 Sep 2018 21:15:39 -0000 1.63
+++ sys/compat/netbsd32/netbsd32_ioctl.h 11 Oct 2018 12:00:06 -0000
@@ -116,6 +116,7 @@ struct netbsd32_bpf_dltlist {
#define BIOCSTCPF32 _IOW('B',114, struct netbsd32_bpf_program)
#define BIOCSUDPF32 _IOW('B',115, struct netbsd32_bpf_program)
#define BIOCGDLTLIST32 _IOWR('B',119, struct netbsd32_bpf_dltlist)
+#define BIOCSRTIMEOUT32 _IOW('B',122, struct netbsd32_timeval)
struct netbsd32_wsdisplay_addscreendata {
----
In the patch, I need preprocessor hacks in order to get along with
IOCTL_STRUCT_CONV_TO() macro in netbsd32_ioctl.h:
https://nxr.netbsd.org/xref/src/sys/compat/netbsd32/netbsd32_ioctl.h#41
>Release-Note:
>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/53666 CVS commit: src/sys/compat/netbsd32
Date: Thu, 11 Oct 2018 11:23:23 -0400
Module Name: src
Committed By: christos
Date: Thu Oct 11 15:23:22 UTC 2018
Modified Files:
src/sys/compat/netbsd32: netbsd32_ioctl.c netbsd32_ioctl.h
Log Message:
PR/53666: Rin Okuyama: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64. Add BIOCSRTIMEOUT32.
To generate a diff of this commit:
cvs rdiff -u -r1.97 -r1.98 src/sys/compat/netbsd32/netbsd32_ioctl.c
cvs rdiff -u -r1.64 -r1.65 src/sys/compat/netbsd32/netbsd32_ioctl.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->pending-pullups
State-Changed-By: rin@NetBSD.org
State-Changed-When: Fri, 12 Oct 2018 03:16:39 +0000
State-Changed-Why:
Thank you christos for rapid response!
Pull-up request is sent as [pullup-8 #1054].
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org, rokuyama@rk.phys.keio.ac.jp
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Subject: re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32 on amd64
Date: Fri, 12 Oct 2018 15:23:38 +1100
thanks for fixing this!
> +#define netbsd32_to_timeval(p, s32p, cmd) netbsd32_to_timeval(p, s32p)
> +#define netbsd32_from_timeval(p, s32p, cmd) netbsd32_from_timeval(p, s32p)
i guess we should make netbsd32_to_timeval() and
netbsd32_from_timeval() take an unused cmd argument.
then your workaround shouldn't be needed.
can you work on this? thanks.
.mrg.
From: "Rin Okuyama" <rin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/53666 CVS commit: src/sys/compat/netbsd32
Date: Fri, 12 Oct 2018 05:06:05 +0000
Module Name: src
Committed By: rin
Date: Fri Oct 12 05:06:05 UTC 2018
Modified Files:
src/sys/compat/netbsd32: netbsd32_ioctl.c
Log Message:
PR kern/53666
Correct misleading names of dummy variables. No binary changes intended.
To generate a diff of this commit:
cvs rdiff -u -r1.98 -r1.99 src/sys/compat/netbsd32/netbsd32_ioctl.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: matthew green <mrg@eterna.com.au>, gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64
Date: Fri, 12 Oct 2018 14:12:01 +0900
Hi,
On 2018/10/12 13:23, matthew green wrote:
>> +#define netbsd32_to_timeval(p, s32p, cmd) netbsd32_to_timeval(p, s32p)
>> +#define netbsd32_from_timeval(p, s32p, cmd) netbsd32_from_timeval(p, s32p)
>
> i guess we should make netbsd32_to_timeval() and
> netbsd32_from_timeval() take an unused cmd argument.
> then your workaround shouldn't be needed.
>
> can you work on this? thanks.
Well, it should be an option. However, we have more than 40 functions
of netbsd32_{to,from}_foo() in netbsd32_conv.h. Do you think all of
them should take an unused cmd argument?
Also, whereas majority of functions in netbsd32_conv.h have form of
void netbsd_to_foo(struct netbsd32_foo *, struct foo *),
some functions are not. For example, netbsd32_to_iovecin() requires
third argument for array length and returns an integer as error:
https://nxr.netbsd.org/xref/src/sys/compat/netbsd32/netbsd32_conv.h#241
These functions cannot be used from netbsd32_ioctl.[ch] in the present
form. Just add a cmd argument for now, and modify netbsd32_ioctl.[ch]
when it becomes really necessary?
If we add a cmd argument for netbsd32_conv.h, netbsd32_{to,from}_foo()
functions in netbsd32_ioctl.c may be moved into netbsd32_conv.h.
Thanks,
rin
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Subject: re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32 on amd64
Date: Fri, 12 Oct 2018 17:04:25 +1100
i also notice that BIOCGRTIMEOUT needs handling too.
From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: matthew green <mrg@eterna.com.au>, gnats-bugs@NetBSD.org,
Paul Goyette <paul@whooppee.com>
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64
Date: Fri, 12 Oct 2018 15:54:40 +0900
On 2018/10/12 15:04, matthew green wrote:
> i also notice that BIOCGRTIMEOUT needs handling too.
Oops, we also need BIOC[GS]ORTIMEOUT when COMPAT_50:
https://nxr.netbsd.org/xref/src/sys/net/bpf.h#132
Is it OK to have #ifdef COMPAT_50 (or equivalently,
#ifdef BIOC[GS]ORTIMEOUT) in netbsd32_ioctl.c?
Anyway, I would like to decide the way before proceeding further.
(1) add unused "u_long cmd" argument to all netbsd32_{to,from}_foo()
functions in netbsd32_conv.h, and move netbsd32_{to,from}_foo() in
netbsd32_ioctl.c to netbsd32_conv.h
or
(2) leave preprocessor mess as is
or
(3) other
Paul, how do you think?
State-Changed-From-To: pending-pullups->analyzed
State-Changed-By: rin@NetBSD.org
State-Changed-When: Fri, 12 Oct 2018 06:56:03 +0000
State-Changed-Why:
We still need more discussion (and coding)
From: Paul Goyette <paul@whooppee.com>
To: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
Cc: matthew green <mrg@eterna.com.au>, gnats-bugs@NetBSD.org,
kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64
Date: Fri, 12 Oct 2018 17:09:23 +0800 (+08)
On Fri, 12 Oct 2018, Rin Okuyama wrote:
> On 2018/10/12 15:04, matthew green wrote:
>> i also notice that BIOCGRTIMEOUT needs handling too.
>
> Oops, we also need BIOC[GS]ORTIMEOUT when COMPAT_50:
>
> https://nxr.netbsd.org/xref/src/sys/net/bpf.h#132
>
> Is it OK to have #ifdef COMPAT_50 (or equivalently,
> #ifdef BIOC[GS]ORTIMEOUT) in netbsd32_ioctl.c?
Oh, joy! Here comes another conflict with the pgoyette-compat
branch!
But YES, for now it would be acceptable to have a #ifdef COMPAT_50
block. I will extract the conditional code and move it into
compat/netbsd32/netbsd32_compat_50.c and provide an appropriate
linkage/hook.
(Actually, this prompted me to look at the existing code on my
branch, and it seems I've still got some clean-up work to do here
for compat32_50_rnd_ioctl() - the "hook" needs to use the new
MP-safe mechanism to prevent the module code from being unloaded
while the code is executing!)
> Anyway, I would like to decide the way before proceeding further.
>
> (1) add unused "u_long cmd" argument to all netbsd32_{to,from}_foo()
> functions in netbsd32_conv.h, and move netbsd32_{to,from}_foo() in
> netbsd32_ioctl.c to netbsd32_conv.h
>
> or
>
> (2) leave preprocessor mess as is
>
> or
>
> (3) other
>
> Paul, how do you think?
Neither 1 nor 2 "feels right" to me. But I don't have any better
choice to offer.
+------------------+--------------------------+----------------------------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+------------------+--------------------------+----------------------------+
From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: Paul Goyette <paul@whooppee.com>
Cc: matthew green <mrg@eterna.com.au>, gnats-bugs@NetBSD.org,
kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64
Date: Mon, 15 Oct 2018 18:55:55 +0900
Sorry for the late reply,
On 2018/10/12 18:09, Paul Goyette wrote:
> On Fri, 12 Oct 2018, Rin Okuyama wrote:
...
>> Is it OK to have #ifdef COMPAT_50 (or equivalently,
>> #ifdef BIOC[GS]ORTIMEOUT) in netbsd32_ioctl.c?
>
> Oh, joy! Here comes another conflict with the pgoyette-compat
> branch!
>
> But YES, for now it would be acceptable to have a #ifdef COMPAT_50
> block. I will extract the conditional code and move it into
> compat/netbsd32/netbsd32_compat_50.c and provide an appropriate
> linkage/hook.
>
> (Actually, this prompted me to look at the existing code on my
> branch, and it seems I've still got some clean-up work to do here
> for compat32_50_rnd_ioctl() - the "hook" needs to use the new
> MP-safe mechanism to prevent the module code from being unloaded
> while the code is executing!)
and sorry for troubling you...
>> Anyway, I would like to decide the way before proceeding further.
>>
>> (1) add unused "u_long cmd" argument to all netbsd32_{to,from}_foo()
>> functions in netbsd32_conv.h, and move netbsd32_{to,from}_foo() in
>> netbsd32_ioctl.c to netbsd32_conv.h
>>
>> or
>>
>> (2) leave preprocessor mess as is
>>
>> or
>>
>> (3) other
>>
>> Paul, how do you think?
>
> Neither 1 nor 2 "feels right" to me. But I don't have any better
> choice to offer.
Hmm, unfortunately, support for BIOC[GS]ORTIMEOUT makes the situation
worse. We have netbsd32_{to,from}_timeval50() in netbsd32_conv.h, and
they convert between netbsd32_timeval50 and timeval, not timeval50!
Apparently, we have too many different kinds of functions that share
the same naming rule :(.
Fix may be made in two steps: (1) minimum fix to HEAD for now and
netbsd-8, in order to reduce conflicts b/w pgoyette-compat branch as
much as possible, then (2) real fix after merge of pgoyette-compat.
However, I'm not sure neither whether "the minimum fix" like this
http://www.netbsd.org/~rin/netbsd32_ioctl_20181015.patch
is acceptable, nor how "the real fix" should be...
Thanks,
rin
From: Paul Goyette <paul@whooppee.com>
To: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
Cc: matthew green <mrg@eterna.com.au>, gnats-bugs@NetBSD.org,
kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64
Date: Mon, 15 Oct 2018 18:04:17 +0800 (+08)
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--0-756126138-1539597608=:19695
Content-Type: TEXT/PLAIN; CHARSET=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-ID: <Pine.NEB.4.64.1810151800131.19695@speedy.whooppee.com>
On Mon, 15 Oct 2018, Rin Okuyama wrote:
>> But YES, for now it would be acceptable to have a #ifdef COMPAT_50
>> block.=C2=A0 I will extract the conditional code and move it into
>> compat/netbsd32/netbsd32_compat_50.c and provide an appropriate
>> linkage/hook.
>>=20
>> (Actually, this prompted me to look at the existing code on my
>> branch, and it seems I've still got some clean-up work to do here
>> for compat32_50_rnd_ioctl() - the "hook" needs to use the new
>> MP-safe mechanism to prevent the module code from being unloaded
>> while the code is executing!)
>
> and sorry for troubling you...
Don't worry about it. This is the risk one takes when working on a=20
branch. I will deal with it.
> Hmm, unfortunately, support for BIOC[GS]ORTIMEOUT makes the situation
> worse. We have netbsd32_{to,from}_timeval50() in netbsd32_conv.h, and
> they convert between netbsd32_timeval50 and timeval, not timeval50!
> Apparently, we have too many different kinds of functions that share
> the same naming rule :(.
>
> Fix may be made in two steps: (1) minimum fix to HEAD for now and
> netbsd-8, in order to reduce conflicts b/w pgoyette-compat branch as
> much as possible, then (2) real fix after merge of pgoyette-compat.
>
> However, I'm not sure neither whether "the minimum fix" like this
>
> http://www.netbsd.org/~rin/netbsd32_ioctl_20181015.patch
>
> is acceptable, nor how "the real fix" should be...
I think we need to decide on what "the real fix" looks like before we=20
decide to use one-step or two-step patch.
+------------------+--------------------------+----------------------------=
+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: =
|
| (Retired) | FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com =
|
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org =
|
+------------------+--------------------------+----------------------------=
+
--0-756126138-1539597608=:19695--
From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: Paul Goyette <paul@whooppee.com>
Cc: matthew green <mrg@eterna.com.au>, gnats-bugs@NetBSD.org,
kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/53666: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64
Date: Mon, 15 Oct 2018 19:28:12 +0900
On 2018/10/15 19:04, Paul Goyette wrote:
...
>> and sorry for troubling you...
>
> Don't worry about it. This is the risk one takes when working on a branch. I will deal with it.
Thank you for your kind words :).
>> Hmm, unfortunately, support for BIOC[GS]ORTIMEOUT makes the situation
>> worse. We have netbsd32_{to,from}_timeval50() in netbsd32_conv.h, and
>> they convert between netbsd32_timeval50 and timeval, not timeval50!
>> Apparently, we have too many different kinds of functions that share
>> the same naming rule :(.
>>
>> Fix may be made in two steps: (1) minimum fix to HEAD for now and
>> netbsd-8, in order to reduce conflicts b/w pgoyette-compat branch as
>> much as possible, then (2) real fix after merge of pgoyette-compat.
>>
>> However, I'm not sure neither whether "the minimum fix" like this
>>
>> http://www.netbsd.org/~rin/netbsd32_ioctl_20181015.patch
>>
>> is acceptable, nor how "the real fix" should be...
>
> I think we need to decide on what "the real fix" looks like before we decide to use one-step or two-step patch.
OK. IMO, the cause of trouble is naming rule violation for
netbsd32_{to,from}_foo() functions; we have (at least) four
different kinds of functions:
(1) Functions in netbsd32_conv.h that take two argument, struct
netbsd32_foo, and struct foo
(2) Functions in netbsd32_conv.h that require additional arguments,
e.g., netbsd32_to_iovecin():
https://nxr.netbsd.org/xref/src/sys/compat/netbsd32/netbsd32_conv.h#241
(3) Functions in netbsd32_conv.h that do *not* convert b/w struct
netbsd32_foo and struct foo, e.g., netbsd32_to_timeval50():
https://nxr.netbsd.org/xref/src/sys/compat/netbsd32/netbsd32_conv.h#72
(4) Functions in netbsd32_ioctl.c that require "u_long cmd" argument.
Note that the most of these functions do not actually use "cmd".
At the moment, I cannot imagine how to resolve this confusion...
Thanks,
rin
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/53666 CVS commit: [netbsd-8] src/sys/compat/netbsd32
Date: Mon, 22 Jul 2019 16:27:30 +0000
Module Name: src
Committed By: martin
Date: Mon Jul 22 16:27:29 UTC 2019
Modified Files:
src/sys/compat/netbsd32 [netbsd-8]: netbsd32_ioctl.c netbsd32_ioctl.h
Log Message:
Pull up following revision(s) (requested by rin in ticket #1054):
sys/compat/netbsd32/netbsd32_ioctl.h: revision 1.65
sys/compat/netbsd32/netbsd32_ioctl.c: revision 1.98
sys/compat/netbsd32/netbsd32_ioctl.c: revision 1.99
PR/53666: Rin Okuyama: tcpdump for i386 does not work with COMPAT_NETBSD32
on amd64. Add BIOCSRTIMEOUT32.
-
Correct misleading names of dummy variables. No binary changes intended.
To generate a diff of this commit:
cvs rdiff -u -r1.89.8.1 -r1.89.8.2 src/sys/compat/netbsd32/netbsd32_ioctl.c
cvs rdiff -u -r1.56.8.1 -r1.56.8.2 src/sys/compat/netbsd32/netbsd32_ioctl.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.