From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3ED00C71123 for ; Sat, 13 Oct 2018 07:34:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0F39F20835 for ; Sat, 13 Oct 2018 07:34:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0F39F20835 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ZenIV.linux.org.uk Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726775AbeJMPKO (ORCPT ); Sat, 13 Oct 2018 11:10:14 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:47676 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725986AbeJMPKO (ORCPT ); Sat, 13 Oct 2018 11:10:14 -0400 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.90_1 #2 (Red Hat Linux)) id 1gBEQJ-00054J-8V; Sat, 13 Oct 2018 07:33:19 +0000 Date: Sat, 13 Oct 2018 08:33:19 +0100 From: Al Viro To: Aleksa Sarai 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: <20181013073319.GS32577@ZenIV.linux.org.uk> References: <20181009070230.12884-1-cyphar@cyphar.com> <20181009070230.12884-2-cyphar@cyphar.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181009070230.12884-2-cyphar@cyphar.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 09, 2018 at 06:02:28PM +1100, Aleksa Sarai 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. It is equivalent to setting the starting point for relative pathnames + setting ->root for LOOKUP_BENEATH, right? But the part in path_init() is too bloody convoluted for its own good. Let me try to translate: > + if (unlikely(flags & LOOKUP_XDEV)) { > + error = dirfd_path_init(nd); > + if (unlikely(error)) > + return ERR_PTR(error); > + } * 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. * if it's an absolute pathname, > if (*s == '/') { ... 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 that if we came here with LOOKUP_XDEV, we'll discard the starting point we'd calculated. > + error = nd_jump_root(nd); > + if (unlikely(error)) > + s = 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 relative pathname (which it is) and, if LOOKUP_BENEATH is also there, set ->root there as well. > + error = dirfd_path_init(nd); > + if (unlikely(error)) > + return ERR_PTR(error); > + } > + return s; > } 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 pathname had been relative. Note that LOOKUP_BENEATH alone is ignored for absolute ones (and with a good reason - it's a no-op on path_init() level in that case). What the hell? It complicates your code and doesn't seem to provide any benefits whatsoever -- you could bloody well have passed 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. As it is, the logics is very hard to follow.