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

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

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