Received: by 2002:ac0:a582:0:0:0:0:0 with SMTP id m2-v6csp1672673imm; Sat, 13 Oct 2018 01:12:33 -0700 (PDT) X-Google-Smtp-Source: ACcGV63T72TmYQFO3JmoTsfXODa38NhS+7qpcswVXgvMDPWKWclHZrGTVtVX9reF1LLzzje/mzFj X-Received: by 2002:a63:de05:: with SMTP id f5-v6mr8432176pgg.292.1539418353805; Sat, 13 Oct 2018 01:12:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1539418353; cv=none; d=google.com; s=arc-20160816; b=Mth8TKFhFnAJXU40vy96zS4IF65MeZSFZU6jlDLxK3aLoak4QzmY2KNBIzuxbQvLWp wnOG199LovL9Ri+eA5rlMbEL7RkdfG4fStpCJcJ4iO7ul+oQkBO3yZSLbABxkxDHnct8 H2S3ukxe8IGZGrlrmhJHac5XDelIt0ZaEGH2XksXUUayAXo9AHtmLARl9labaN3doouw CtPMRF6fs0hNqvQ73zArhpcx8f1VtXPDuEnI6FwF0j+GWlgoc7Xrh7MbyejvmkcWTIt+ U5gfWAlVJhc5tns0giPqHamC10+QBxfQfTiHJMk3DDbjhMCq536kMFOBc1wHd0JLPghN cvlw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=iNORBW0QXw9tPrLVr00s87yPeyjCens3O4jMnCf7RD4=; b=Iplk+jS+dw5XHuqCoSj6Entm6VlnIQEBkwS6l6U3CQMFFRC2IRJgZt66zNLsEChKUN 74tCecbooIqMy5QnnSwvowdx794EkjYWtFIOiaizdMY/dH+0mHa+VnGBwxzFsfTtAbz5 ATin/CbOTGmLEfyQZJ+QUUCA0h1rK/qyxGKbI1MATlbJGFOGyvhhUY0V61+aAd++uYsk +pD20+OoRykJWv6eoeQm4xSj8hNsWdRB/J6vAoIEJ3yRyrd3CRmHXV8eem5AXrnQ39mA Upfh1LQSzhU4yrmNcz74tSji2G3SysmyKN0mJ7TftFV8IvaSZvoVkepoxIUbwAnFEz4N daxw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w22-v6si3934120plk.377.2018.10.13.01.12.18; Sat, 13 Oct 2018 01:12:33 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726837AbeJMPpr (ORCPT + 99 others); Sat, 13 Oct 2018 11:45:47 -0400 Received: from mx2.mailbox.org ([80.241.60.215]:15486 "EHLO mx2.mailbox.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726647AbeJMPpr (ORCPT ); Sat, 13 Oct 2018 11:45:47 -0400 Received: from smtp2.mailbox.org (unknown [IPv6:2001:67c:2050:105:465:1:2:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx2.mailbox.org (Postfix) with ESMTPS id 54858A1055; Sat, 13 Oct 2018 10:09:32 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp2.mailbox.org ([80.241.60.241]) by spamfilter03.heinlein-hosting.de (spamfilter03.heinlein-hosting.de [80.241.56.117]) (amavisd-new, port 10030) with ESMTP id XCZxRiz5c-OY; Sat, 13 Oct 2018 10:09:29 +0200 (CEST) Date: Sat, 13 Oct 2018 19:09:08 +1100 From: Aleksa Sarai To: Al Viro Cc: Eric Biederman , Christian Brauner , Jeff Layton , "J. Bruce Fields" , Arnd Bergmann , Andy Lutomirski , David Howells , Jann Horn , Tycho Andersen , David Drysdale , dev@opencontainers.org, containers@lists.linux-foundation.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org Subject: Re: [PATCH v3 1/3] namei: implement O_BENEATH-style AT_* flags Message-ID: <20181013080907.yqpuy3zbbfe46gm4@ryuk> References: <20181009070230.12884-1-cyphar@cyphar.com> <20181009070230.12884-2-cyphar@cyphar.com> <20181013073319.GS32577@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dst6zextluqynn7a" Content-Disposition: inline In-Reply-To: <20181013073319.GS32577@ZenIV.linux.org.uk> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --dst6zextluqynn7a Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 2018-10-13, Al Viro wrote: > First of all, dirfd_path_init() part should be in a separate commit. And= I'm > really not happy with the logics in there. dirfd_path_init() itself is > kinda-sorta reasonable. Sure, I can do that. > It is equivalent to setting the starting point for > relative pathnames + setting ->root for LOOKUP_BENEATH, right? Right. > But the part in path_init() is too bloody convoluted for its own good. L= et me > try to translate: >=20 > > + if (unlikely(flags & LOOKUP_XDEV)) { > > + error =3D dirfd_path_init(nd); > > + if (unlikely(error)) > > + return ERR_PTR(error); > > + } >=20 > * if LOOKUP_XDEV is set, set the starting point as if it was a relative > pathname. If LOOKUP_BENEATH was set as well, set ->root to the same > point. Right. This is for two reasons (though if you disagree with these semantics we can change this as well): 1. It's not clear to me whether openat(somefd->"/", "/tmp", O_XDEV) should return an -EXDEV or completely ignore the starting point. Same argument with AT_FDCWD. I opted to make it so that the starting point has to be on the same mountpoint, but I totally understand if you feel this is insane -- and I'd be happy to change it. The real problem comes from (2). 2. AT_THIS_ROOT chroot-scope absolute paths, and so in the second patch LOOKUP_CHROOT also triggers this codepath. The main argument for this semantic is somewhat elaborated in the cover letter -- but the short version is because AT_THIS_ROOT has to chroot-scope absolute symlinks it would be somewhat strange if it didn't scope absolute paths you give it -- otherwise it could either be a footgun or would require always returning -EXDEV here. Though, as above, if you feel that the current semantics (absolute paths override whatever dirfd you give), then -EXDEV is the alternative I would pitch. > * if it's an absolute pathname,=20 > > if (*s =3D=3D '/') { > ... and we hadn't come here with LOOKUP_XDEV + LOOKUP_BENEATH, set ->root. > > + if (likely(!nd->root.mnt)) > > + set_root(nd); > * if it's an absolute pathname, set the starting point to ->root. Note t= hat > if we came here with LOOKUP_XDEV, we'll discard the starting point we'd > calculated. We wouldn't discard it -- nd_jump_root() will check whether a mount crossing was implied here (otherwise an absolute symlink could cause you to cross a mountpoint). But as above, if you'd prefer that absolute paths disable all dirfd handling (as is the case now), I can remove this semantic. > > + error =3D nd_jump_root(nd); > > + if (unlikely(error)) > > + s =3D ERR_PTR(error); > > return s; > > } > > + if (likely(!nd->path.mnt)) { > * if we didn't have LOOKUP_XDEV, set the starting point as if it was a re= lative > pathname (which it is) and, if LOOKUP_BENEATH is also there, set ->root t= here > as well. > > + error =3D dirfd_path_init(nd); > > + if (unlikely(error)) > > + return ERR_PTR(error); > > + } > > + return s; > > } >=20 > Pardon me, but... huh? The reason for your two calls of dirfd_path_init(= ) is, > AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and > LOOKUP_BENEATH at the same time. That combination is treated as if the p= athname > had been relative. Note that LOOKUP_BENEATH alone is ignored for absolut= e ones > (and with a good reason - it's a no-op on path_init() level in that case). >=20 > What the hell? It complicates your code and doesn't seem to provide any = benefits > whatsoever The reasoning for this is because of how AT_THIS_ROOT uses both of these codepaths (it causes dirfd_path_init() to be called before the absolute check, and also causes ->root to be set). I wrote the features in parallel and then split out the code for AT_THIS_ROOT so it could be discussed separately (and so removing it if it was rejected would be simpler). But unfortunately this does result in the dirfd_path_init() code looking completely superfluous without seeing the second patch. > -- you could bloody well have passed the relative pathname to start with. (I think you mean always doing dirfd_path_init() first here?) Right, but I didn't want to discard nd->path unnecessarily -- if we do all of the code to grab AT_FDCWD and then it is completely unused (not even in the AT_XDEV sense of "unused") it seems like a waste. Did I misunderstand your suggestion? Were you referring to userspace just being able to "[pass] the relative pathname to start with"? > IDGI... Without that kludge it becomes simply "do as we currently do for= absolute > pathnames, call dirfd_path_init() for relative ones". And I would argue = that > taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() = (relative) > case would be a good idea. Right, I could definitely do that -- though for AT_THIS_ROOT we'd duplicate the ->root setting in both places. > As it is, the logics is very hard to follow. Sorry about that. Would you prefer if the two patches (AT_BENEATH family and AT_THIS_ROOT) were sent as a single patch -- with the dirfd_path_init() code split out? Or that the second patch do all of the structural changes to refactor dirfd_path_init() usage? --=20 Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH --dst6zextluqynn7a Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEXzbGxhtUYBJKdfWmnhiqJn3bjbQFAlvBqCEACgkQnhiqJn3b jbQxqg/+Kh8EJI8aqy0Yr1G79viNi3O0TUKOL0miieS39gPO12GEjHX/UJ43G6Yx 40vteAcFl08N1v6UIFmNfrw3N2h6Nz+Ss+1vCPF8GjQZyiGrRfSn0dTFgPDd9rOp oJg+1fDjrPOPBwC8f1gsme6slLm9xlk+LRXZCi4yltttiDc8nydWpG5/Zn61thTL /dw0ZjwZSIERA401EYpgtnsrdxcyAKh0OwPm2OGHvcaHfn950rYgvIRwptY2Xgzf 6kIJDX1bc/m3hWyQ1PLvZWdws63+7duT9b8ojtZqRrko9+M1tBn+skp1KpqfL6+q Gdb+O2hJELJPBH2fgXfi5jDxGNuamJeNEkiabBEGGviGESGLOnloEtsU4v63gRC9 Md0V8F6Y8n0o5XzzgXh/dRAAvZ2cdd9JD4sJzo4NIW934ukkzJAI28zYuWdA36sy qGA52jro88jukSsef5g/F+4f5OzT8U+bVhtwnAZGBHxaLIImn3j5595+mfR7nVmD RrlVn2/OZ66Z9rboXGfeWm7SGCSagobQCUD6JY973Zx4XHOr/kwAkokFMJotbmnq CNLzVsU9Idy/Z9O+qCTDrQgjQgMWI0YUubvJbGe9Gh9jOphDH71wskUsC8BahIqK QPzRQPs+RCZ841+KCg+vhyQ+9SECubA8gQ87UNOEsSN3ZIgIcDE= =SNGq -----END PGP SIGNATURE----- --dst6zextluqynn7a--