NetBSD Problem Report #54946
From www@netbsd.org Fri Feb 7 22:46:54 2020
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 7105C1A9213
for <gnats-bugs@gnats.NetBSD.org>; Fri, 7 Feb 2020 22:46:54 +0000 (UTC)
Message-Id: <20200207224653.631BA1A921A@mollari.NetBSD.org>
Date: Fri, 7 Feb 2020 22:46:53 +0000 (UTC)
From: kevans@FreeBSD.org
Reply-To: kevans@FreeBSD.org
To: gnats-bugs@NetBSD.org
Subject: O_SEARCH tests: expect revoke +x failure on NFS
X-Send-Pr-Version: www-1.0
>Number: 54946
>Category: misc
>Synopsis: O_SEARCH tests: expect revoke +x failure on NFS
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: misc-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Fri Feb 07 22:50:00 +0000 2020
>Closed-Date: Sat Feb 08 19:59:30 +0000 2020
>Last-Modified: Sat Feb 08 20:00:03 +0000 2020
>Originator: Kyle Evans
>Release:
>Organization:
>Environment:
>Description:
O_SEARCH semantics are very clear, here: faccessat() should not be checking for the executable bit on the atfd if it was opened with O_SEARCH. We've since realized that NFS cannot possibly honor these semantics, because it will re-check at lookup time every time and there is no way to indicate in a safe or portable fashion that it should avoid doing so -- the NFS server typically won't know that this specific fd was initially opened with O_SEARCH, and it certainly won't know that the directory did have the executable bit set at the time of open(, O_SEARCH).
There's still some value in attempting the test, though- this is a newly created file as of this test, so any kind of success would be due to bogus caching.
This is completely untested on NetBSD, but the statvfs use looks to be correct based on the documentation I looked at...
>How-To-Repeat:
>Fix:
Index: tests/lib/libc/c063/t_o_search.c
===================================================================
RCS file: /cvsroot/src/tests/lib/libc/c063/t_o_search.c,v
retrieving revision 1.9
diff -u -r1.9 t_o_search.c
--- tests/lib/libc/c063/t_o_search.c 6 Feb 2020 12:18:06 -0000 1.9
+++ tests/lib/libc/c063/t_o_search.c 7 Feb 2020 22:40:57 -0000
@@ -34,6 +34,11 @@
#include <atf-c.h>
#include <sys/types.h>
+#ifdef __FreeBSD__
+#include <sys/mount.h>
+#else
+#include <sys/statvfs.h>
+#endif
#include <sys/stat.h>
#include <dirent.h>
@@ -322,6 +327,23 @@
/* Drop permissions. The kernel must still not check the exec bit. */
ATF_REQUIRE(chmod(DIR, 0000) == 0);
+ {
+ const char *fstypename;
+#ifdef __FreeBSD__
+ struct statfs st;
+
+ fstatfs(dfd, &st);
+ fstypename = st.f_fstypename;
+#else
+ struct statvfs vst;
+
+ fstatvfs(dfd, &vst);
+ fstypename = vst.f_fstypename;
+#endif
+ if (strcmp(fstypename, "nfs") == 0)
+ atf_tc_expect_fail(
+ "NFS protocol cannot observe O_SEARCH semantics");
+ }
ATF_REQUIRE(fstatat(dfd, BASEFILE, &sb, 0) == 0);
ATF_REQUIRE(close(dfd) == 0);
>Release-Note:
>Audit-Trail:
From: Kamil Rytarowski <n54@gmx.com>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: misc/54946: O_SEARCH tests: expect revoke +x failure on NFS
Date: Sat, 8 Feb 2020 12:41:14 +0100
On 07.02.2020 23:50, kevans@FreeBSD.org wrote:
>> Number: 54946
>> Category: misc
>> Synopsis: O_SEARCH tests: expect revoke +x failure on NFS
>> Confidential: no
>> Severity: non-critical
>> Priority: medium
>> Responsible: misc-bug-people
>> State: open
>> Class: sw-bug
>> Submitter-Id: net
>> Arrival-Date: Fri Feb 07 22:50:00 +0000 2020
>> Originator: Kyle Evans
>> Release:
>> Organization:
>> Environment:
>> Description:
> O_SEARCH semantics are very clear, here: faccessat() should not be check=
ing for the executable bit on the atfd if it was opened with O_SEARCH. We'=
ve since realized that NFS cannot possibly honor these semantics, because =
it will re-check at lookup time every time and there is no way to indicate=
in a safe or portable fashion that it should avoid doing so -- the NFS se=
rver typically won't know that this specific fd was initially opened with =
O_SEARCH, and it certainly won't know that the directory did have the exec=
utable bit set at the time of open(, O_SEARCH).
>
> There's still some value in attempting the test, though- this is a newly=
created file as of this test, so any kind of success would be due to bogu=
s caching.
>
> This is completely untested on NetBSD, but the statvfs use looks to be c=
orrect based on the documentation I looked at...
>> How-To-Repeat:
>
>> Fix:
> Index: tests/lib/libc/c063/t_o_search.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /cvsroot/src/tests/lib/libc/c063/t_o_search.c,v
> retrieving revision 1.9
> diff -u -r1.9 t_o_search.c
> --- tests/lib/libc/c063/t_o_search.c 6 Feb 2020 12:18:06 -0000 =
1.9
> +++ tests/lib/libc/c063/t_o_search.c 7 Feb 2020 22:40:57 -0000
> @@ -34,6 +34,11 @@
> #include <atf-c.h>
>
> #include <sys/types.h>
> +#ifdef __FreeBSD__
> +#include <sys/mount.h>
> +#else
> +#include <sys/statvfs.h>
> +#endif
> #include <sys/stat.h>
>
> #include <dirent.h>
We can include both headers for both BSDs without ifdefs.
> @@ -322,6 +327,23 @@
>
> /* Drop permissions. The kernel must still not check the exec bi=
t. */
> ATF_REQUIRE(chmod(DIR, 0000) =3D=3D 0);
> + {
> + const char *fstypename;
> +#ifdef __FreeBSD__
> + struct statfs st;
> +
> + fstatfs(dfd, &st);
> + fstypename =3D st.f_fstypename;
> +#else
> + struct statvfs vst;
> +
> + fstatvfs(dfd, &vst);
> + fstypename =3D vst.f_fstypename;
> +#endif
> + if (strcmp(fstypename, "nfs") =3D=3D 0)
> + atf_tc_expect_fail(
> + "NFS protocol cannot observe O_SEARCH semant=
ics");
> + }
> ATF_REQUIRE(fstatat(dfd, BASEFILE, &sb, 0) =3D=3D 0);
>
> ATF_REQUIRE(close(dfd) =3D=3D 0);
>
Is viable for FreeBSD to switch to POSIX interface statvfs and share the
same code paths? This way we can avoid ifdefing around and get better
compat in 3rd party code.
From: Kyle Evans <kevans@freebsd.org>
To: gnats-bugs@netbsd.org
Cc: misc-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: misc/54946: O_SEARCH tests: expect revoke +x failure on NFS
Date: Sat, 8 Feb 2020 10:36:40 -0600
On Sat, Feb 8, 2020 at 5:45 AM Kamil Rytarowski <n54@gmx.com> wrote:
> On 07.02.2020 23:50, kevans@FreeBSD.org wrote:
> >> Number: 54946
> >> Category: misc
> >> Synopsis: O_SEARCH tests: expect revoke +x failure on NFS
> >> Confidential: no
> >> Severity: non-critical
> >> Priority: medium
> >> Responsible: misc-bug-people
> >> State: open
> >> Class: sw-bug
> >> Submitter-Id: net
> >> Arrival-Date: Fri Feb 07 22:50:00 +0000 2020
> >> Originator: Kyle Evans
> >> Release:
> >> Organization:
> >> Environment:
> >> Description:
> > O_SEARCH semantics are very clear, here: faccessat() should not be check=
> ing for the executable bit on the atfd if it was opened with O_SEARCH. We'=
> ve since realized that NFS cannot possibly honor these semantics, because =
> it will re-check at lookup time every time and there is no way to indicate=
> in a safe or portable fashion that it should avoid doing so -- the NFS se=
> rver typically won't know that this specific fd was initially opened with =
> O_SEARCH, and it certainly won't know that the directory did have the exec=
> utable bit set at the time of open(, O_SEARCH).
> >
> > There's still some value in attempting the test, though- this is a newly=
> created file as of this test, so any kind of success would be due to bogu=
> s caching.
> >
> > This is completely untested on NetBSD, but the statvfs use looks to be c=
> orrect based on the documentation I looked at...
> >> How-To-Repeat:
> >
> >> Fix:
> > Index: tests/lib/libc/c063/t_o_search.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > RCS file: /cvsroot/src/tests/lib/libc/c063/t_o_search.c,v
> > retrieving revision 1.9
> > diff -u -r1.9 t_o_search.c
> > --- tests/lib/libc/c063/t_o_search.c 6 Feb 2020 12:18:06 -0000 =
> 1.9
> > +++ tests/lib/libc/c063/t_o_search.c 7 Feb 2020 22:40:57 -0000
> > @@ -34,6 +34,11 @@
> > #include <atf-c.h>
> >
> > #include <sys/types.h>
> > +#ifdef __FreeBSD__
> > +#include <sys/mount.h>
> > +#else
> > +#include <sys/statvfs.h>
> > +#endif
> > #include <sys/stat.h>
> >
> > #include <dirent.h>
>
> We can include both headers for both BSDs without ifdefs.
>
Sure- will fix.
> > @@ -322,6 +327,23 @@
> >
> > /* Drop permissions. The kernel must still not check the exec bi=
> t. */
> > ATF_REQUIRE(chmod(DIR, 0000) =3D=3D 0);
> > + {
> > + const char *fstypename;
> > +#ifdef __FreeBSD__
> > + struct statfs st;
> > +
> > + fstatfs(dfd, &st);
> > + fstypename =3D st.f_fstypename;
> > +#else
> > + struct statvfs vst;
> > +
> > + fstatvfs(dfd, &vst);
> > + fstypename =3D vst.f_fstypename;
> > +#endif
> > + if (strcmp(fstypename, "nfs") =3D=3D 0)
> > + atf_tc_expect_fail(
> > + "NFS protocol cannot observe O_SEARCH semant=
> ics");
> > + }
> > ATF_REQUIRE(fstatat(dfd, BASEFILE, &sb, 0) =3D=3D 0);
> >
> > ATF_REQUIRE(close(dfd) =3D=3D 0);
> >
>
> Is viable for FreeBSD to switch to POSIX interface statvfs and share the
> same code paths? This way we can avoid ifdefing around and get better
> compat in 3rd party code.
>
Unfortunately I think i'd get some pushback to trying to extend the
statvfs interface to provide f_fstypename; our manpage for statvfs(3)
[0] describes it as filling the buffer pointed to with garbage that
may resemble filesystem statistics, and has read like this since it
was introduced in 2002 -- and we had never extended it with the
fstypename. Instead, we doubled down on the distinctly different
fstatfs() that adds a versioning aspect to the struct.
I think we're kinda goofed no matter what we do, because NetBSD's
fstatvfs is also not necessarily portable given that the fstypename is
a NetBSD-extension to the POSIX-specified struct statvfs. =-(
Thanks,
Kyle Evans
[0] https://www.freebsd.org/cgi/man.cgi?query=statvfs&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html
From: Kyle Evans <kevans@freebsd.org>
To:
Cc: gnats-bugs@netbsd.org, misc-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Subject: Re: misc/54946: O_SEARCH tests: expect revoke +x failure on NFS
Date: Sat, 8 Feb 2020 12:25:11 -0600
On Sat, Feb 8, 2020 at 10:36 AM Kyle Evans <kevans@freebsd.org> wrote:
>
> On Sat, Feb 8, 2020 at 5:45 AM Kamil Rytarowski <n54@gmx.com> wrote:
> > On 07.02.2020 23:50, kevans@FreeBSD.org wrote:
> > >> Number: 54946
> > >> Category: misc
> > >> Synopsis: O_SEARCH tests: expect revoke +x failure on NFS
> > >> Confidential: no
> > >> Severity: non-critical
> > >> Priority: medium
> > >> Responsible: misc-bug-people
> > >> State: open
> > >> Class: sw-bug
> > >> Submitter-Id: net
> > >> Arrival-Date: Fri Feb 07 22:50:00 +0000 2020
> > >> Originator: Kyle Evans
> > >> Release:
> > >> Organization:
> > >> Environment:
> > >> Description:
> > > O_SEARCH semantics are very clear, here: faccessat() should not be check=
> > ing for the executable bit on the atfd if it was opened with O_SEARCH. We'=
> > ve since realized that NFS cannot possibly honor these semantics, because =
> > it will re-check at lookup time every time and there is no way to indicate=
> > in a safe or portable fashion that it should avoid doing so -- the NFS se=
> > rver typically won't know that this specific fd was initially opened with =
> > O_SEARCH, and it certainly won't know that the directory did have the exec=
> > utable bit set at the time of open(, O_SEARCH).
> > >
> > > There's still some value in attempting the test, though- this is a newly=
> > created file as of this test, so any kind of success would be due to bogu=
> > s caching.
> > >
> > > This is completely untested on NetBSD, but the statvfs use looks to be c=
> > orrect based on the documentation I looked at...
> > >> How-To-Repeat:
> > >
> > >> Fix:
> > > Index: tests/lib/libc/c063/t_o_search.c
> > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > RCS file: /cvsroot/src/tests/lib/libc/c063/t_o_search.c,v
> > > retrieving revision 1.9
> > > diff -u -r1.9 t_o_search.c
> > > --- tests/lib/libc/c063/t_o_search.c 6 Feb 2020 12:18:06 -0000 =
> > 1.9
> > > +++ tests/lib/libc/c063/t_o_search.c 7 Feb 2020 22:40:57 -0000
> > > @@ -34,6 +34,11 @@
> > > #include <atf-c.h>
> > >
> > > #include <sys/types.h>
> > > +#ifdef __FreeBSD__
> > > +#include <sys/mount.h>
> > > +#else
> > > +#include <sys/statvfs.h>
> > > +#endif
> > > #include <sys/stat.h>
> > >
> > > #include <dirent.h>
> >
> > We can include both headers for both BSDs without ifdefs.
> >
>
> Sure- will fix.
>
> > > @@ -322,6 +327,23 @@
> > >
> > > /* Drop permissions. The kernel must still not check the exec bi=
> > t. */
> > > ATF_REQUIRE(chmod(DIR, 0000) =3D=3D 0);
> > > + {
> > > + const char *fstypename;
> > > +#ifdef __FreeBSD__
> > > + struct statfs st;
> > > +
> > > + fstatfs(dfd, &st);
> > > + fstypename =3D st.f_fstypename;
> > > +#else
> > > + struct statvfs vst;
> > > +
> > > + fstatvfs(dfd, &vst);
> > > + fstypename =3D vst.f_fstypename;
> > > +#endif
> > > + if (strcmp(fstypename, "nfs") =3D=3D 0)
> > > + atf_tc_expect_fail(
> > > + "NFS protocol cannot observe O_SEARCH semant=
> > ics");
> > > + }
> > > ATF_REQUIRE(fstatat(dfd, BASEFILE, &sb, 0) =3D=3D 0);
> > >
> > > ATF_REQUIRE(close(dfd) =3D=3D 0);
> > >
> >
> > Is viable for FreeBSD to switch to POSIX interface statvfs and share the
> > same code paths? This way we can avoid ifdefing around and get better
> > compat in 3rd party code.
> >
>
> Unfortunately I think i'd get some pushback to trying to extend the
> statvfs interface to provide f_fstypename; our manpage for statvfs(3)
> [0] describes it as filling the buffer pointed to with garbage that
> may resemble filesystem statistics, and has read like this since it
> was introduced in 2002 -- and we had never extended it with the
> fstypename. Instead, we doubled down on the distinctly different
> fstatfs() that adds a versioning aspect to the struct.
>
> I think we're kinda goofed no matter what we do, because NetBSD's
> fstatvfs is also not necessarily portable given that the fstypename is
> a NetBSD-extension to the POSIX-specified struct statvfs. =-(
>
> Thanks,
>
> Kyle Evans
>
> [0] https://www.freebsd.org/cgi/man.cgi?query=statvfs&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html
Take two; this one unconditionally includes both headers. There's only
a small #ifdef section near the top to just redefine statvfs as
fstatfs on FreeBSD and the rest is relatively readable inline.
Thoughts?
My mail client attempted to butcher the diff, so I've uploaded it
here: https://people.freebsd.org/~kevans/nfs-search.diff
State-Changed-From-To: open->closed
State-Changed-By: kamil@NetBSD.org
State-Changed-When: Sat, 08 Feb 2020 20:59:30 +0100
State-Changed-Why:
Patch applied in src/tests/lib/libc/c063/t_o_search.c r1.10
From: "Kamil Rytarowski" <kamil@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54946 CVS commit: src/tests/lib/libc/c063
Date: Sat, 8 Feb 2020 19:58:37 +0000
Module Name: src
Committed By: kamil
Date: Sat Feb 8 19:58:37 UTC 2020
Modified Files:
src/tests/lib/libc/c063: t_o_search.c
Log Message:
O_SEARCH tests: expect revoke +x failure on NFS
Patch by Kyle Evans (FreeBSD)
PR misc/54946
To generate a diff of this commit:
cvs rdiff -u -r1.9 -r1.10 src/tests/lib/libc/c063/t_o_search.c
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.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home