Re: misc/54946: O_SEARCH tests: expect revoke +x failure on NFS

[Kyle Evans <kevans%freebsd.org@localhost>]

The following reply was made to PR misc/54946; it has been noted by GNATS.

From: Kyle Evans <kevans%freebsd.org@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: misc-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
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@localhost> wrote:
 >  On 07.02.2020 23:50, kevans%FreeBSD.org@localhost 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