Message-ID: <20200229152656.gwu7wbqd32liwjye@yavin>
Date:   Sun, 1 Mar 2020 02:26:56 +1100
From:   Aleksa Sarai <cyphar@...har.com>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     David Howells <dhowells@...hat.com>, linux-api@...r.kernel.org,
        viro@...iv.linux.org.uk, metze@...ba.org,
        torvalds@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, fweimer@...hat.com
Subject: Re: Have RESOLVE_* flags superseded AT_* flags for new syscalls?

On 2020-02-28, Christian Brauner <christian.brauner@...ntu.com> wrote:
> [Cc Florian since that ends up on libc's table sooner or later...]
> 
> On Fri, Feb 28, 2020 at 02:53:32PM +0000, David Howells wrote:
> > 	
> > I've been told that RESOLVE_* flags, which can be found in linux/openat2.h,
> > should be used instead of the equivalent AT_* flags for new system calls.  Is
> > this the case?
> 
> Imho, it would make sense to use RESOLVE_* flags for new system calls
> and afair this was the original intention.

Yes, RESOLVE_ flags would ideally be usable with all new system calls
(though only where it makes sense, obviously). This would make it much
easier for userspace to safely resolve paths without having to go
through several levels of O_PATH fuckery.

The "openat2.h" name was honestly a completely arbitrary decision.

> So we either end up adding new AT_* flags mirroring the new RESOLVE_*
> flags or we end up adding new RESOLVE_* flags mirroring parts of AT_*
> flags. And if that's a possibility I vote for RESOLVE_* flags going
> forward. The have better naming too imho.

I can see the argument for merging AT_ flags into RESOLVE_ flags (fewer
flag arguments for syscalls is usually a good thing) ... but I don't
really like it. There are a couple of problems right off the bat:

 * The prefix RESOLVE_ implies that the flag is specifically about path
   resolution. While you could argue that AT_EMPTY_PATH is at least
   *related* to path resolution, flags like AT_REMOVEDIR and
   AT_RECURSIVE aren't.

 * That point touches on something I see as a more fundamental problem
   in the AT_ flags -- they were intended to be generic flags for all of
   the ...at(2) syscalls. But then AT_ grew things like AT_STATX_ and
   AT_REMOVEDIR (both of which are necessary features to have for their
   respective syscalls, but now those flag bits are dead for other
   syscalls -- not to mention the whole AT_SYMLINK_{NO,}FOLLOW thing).

 * While the above might be seen as minor quibbles, the really big
   issue is that even the flags which are "similar" (AT_SYMLINK_NOFOLLOW
   and RESOLVE_NO_SYMLINKS) have different semantics (by design -- in my
   view, AT_SYMLINK_{NO,}FOLLOW / O_NOFOLLOW / lstat(2) has always had
   the wrong semantics if the intention was to be a way to safely avoid
   resolving symlinks).

But maybe I'm just overthinking what a merge of AT_ and RESOLVE_ would
look like -- would it on.

> An argument against this could be that we might end up causing more
> confusion for userspace due to yet another set of flags. But maybe this
> isn't an issue as long as we restrict RESOLVE_* flags to new syscalls.
> When we introduce a new syscall userspace will have to add support for
> it anyway.
> 
> > 
> > If so, should we comment them as being deprecated in the header file?  And
> > should they be in linux/fcntl.h rather than linux/openat2.h?
> > 
> > Also:
> > 
> >  (*) It should be noted that the RESOLVE_* flags are not a superset of the
> >      AT_* flags (there's no equivalent of AT_NO_AUTOMOUNT for example).
> 
> That's true but it seems we could just add e.g. RESOLVE_NO_AUTOMOUNT as
> soon as we have a new syscall showing up that needs it or we have an
> existing syscall (e.g. openat2()) that already uses RESOLVE_* flags and
> needs it?

RESOLVE_NO_AUTOMOUNT is on the roadmap for openat2() -- I mentioned it
as future work in the cover letter. :P

But see my above concerns about merging AT_ and RESOLVE_ flags. The
semantic disconnect between AT_ and RESOLVE_ (which is most obvious with
AT_SYMLINK_NOFOLLOW) also exists for AT_NO_AUTOMOUNT.

> >  (*) It has been suggested that AT_SYMLINK_NOFOLLOW should be the default, but
> >      only RESOLVE_NO_SYMLINKS exists.
> 
> I'd be very much in favor of not following symlinks being the default.
> That's usually a source of a lot of security issues.
> And since no kernel with openat2() has been released there's still time
> to switch it and with openat2() being a new syscall it won't hurt if it
> has new semantics; I mean it deviates from openat() - intentionally -
> already.

I agree in principle, but the problem is that if we want to add new
RESOLVE_ flags you end up with half (or fewer) of the flags being opt-in
with the rest necessarily being opt-out (since the flag not being set
needs to be the old behaviour).

There's also a slight ugliness with RESOLVE_SYMLINKS|RESOLVE_MAGICLINKS
-- should you have to specify both or should RESOLVE_MAGICLINKS imply
RESOLVE_SYMLINKS but only for magic-links. (Is allowing magic-links but
not symlinks even a sane thing to do?)

Also I have a very strong feeling people won't like RESOLVE_XDEV nor
RESOLVE_SYMLINKS being opt-in -- lots of systems use bind-mounts and
symlinks in system paths and developers might not be aware of this.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists