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

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

On Sat, Feb 8, 2020 at 10:36 AM Kyle Evans <kevans%freebsd.org@localhost> wrote:
>
> 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

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