Received: by 2002:a05:6a10:a841:0:0:0:0 with SMTP id d1csp59829pxy; Wed, 21 Apr 2021 18:29:31 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwhQU35w3gKspjgrqV9NlFYOdJbq/IvgOxNfsiTuYI1tAy7Y0+fRbP1ru64RGvGHmLiM0N5 X-Received: by 2002:a63:5301:: with SMTP id h1mr935792pgb.109.1619054971563; Wed, 21 Apr 2021 18:29:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1619054971; cv=none; d=google.com; s=arc-20160816; b=Scfdeo1xEyel3mWavnyjoz8PijImipIZ5z3EMJmWJktDEOzM4PTF3MR0iC5HKJcan7 lARVNF+C/qXRiir4+s9txcqeGvFZIm+iLIs2Se/iZX30L/9nObuhSvS6bMa7RS7bL3ck qp3NRXypu3aiA0i7klJckHTY3u8QlRtQixG31DsJbsTIXXDLv9Hv4EA4c/LLOG3wj4h1 klutqhdKoDYTF7xBRz7uxbw+m0wn80ZYSAmeJfmgyN8B1iNKXkMuQI6Plk55dUGVkx22 6EpIVF4L6L+vLng7SkaTHT7sXqnRCUlBd/j1xaQyamCQweqO3xI2vi/ByOtZUm9/Dxvm ivFQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=mjeTptpVRQKd8eJ9YpoO4cSXIwH3yt1hKUuvdlKvrfY=; b=D64m/SdqwnxVKTNl9NpfIoHAsBL2TVDyo0lrARtl8/w3IxNgoZtwAARR0rVwuj689V 16qB6QQFRst90w19JeGii2MJ+mvgD0v9PlfHv3XwGDjxc0xOqI7MmF33fAiWtK/2J3o0 FVbtx2D+WTtRYcB9HRc+W7CIdGYdTzq8dEYX7rEDaOvgyPe/i0UmtfHKNcHLnpdAxX0Z YX9ejfC71BFLwXx3cgXnkCpI+V2xwyVurWNvV+9HdTLUxHcCHP40QFit1GwukX4i9smp cz/k3JgnjnACbMUnxzmLJENeARzkTzB6amp40WiytG+WHWlS9KqgliVR82WURF8IMWkI QyQQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@tyhicks-com.20150623.gappssmtp.com header.s=20150623 header.b=weL6F9nv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id 6si1230697ple.402.2021.04.21.18.29.20; Wed, 21 Apr 2021 18:29:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@tyhicks-com.20150623.gappssmtp.com header.s=20150623 header.b=weL6F9nv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343720AbhDUXdZ (ORCPT + 99 others); Wed, 21 Apr 2021 19:33:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36614 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234681AbhDUXdZ (ORCPT ); Wed, 21 Apr 2021 19:33:25 -0400 Received: from mail-oi1-x22e.google.com (mail-oi1-x22e.google.com [IPv6:2607:f8b0:4864:20::22e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B3E0C06174A for ; Wed, 21 Apr 2021 16:32:50 -0700 (PDT) Received: by mail-oi1-x22e.google.com with SMTP id n184so18047267oia.12 for ; Wed, 21 Apr 2021 16:32:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tyhicks-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=mjeTptpVRQKd8eJ9YpoO4cSXIwH3yt1hKUuvdlKvrfY=; b=weL6F9nvlqhleHju6lWz+bK16A/7SxLEiiw9sCILjkyNIoNv4G2Zs019TBhdF2P136 g4HvNawsB8Gu2wiHZKky7mtcuLBBy0QdXeQxlkMckghvFIdR/kd61C9GYvqd6REs98Og WZiLIXojk0zvPYqa7HuDn6U4mwhPVeiKQlCBsb3A16ivVci8UJ72cowl5Co3wh3PTv+G RJVJvnkzdznBXLnJsHxDkGFHnpk92l0IRhfISPDgoMLBvoL7YmviL2sObpwWRqxmrGYc W9o6lbUE0h6AukyIUr0sKjWkn3To7hwm6k99GhvFnOD2zyTyuxgo016FZpt+at2kpMn0 V9vg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=mjeTptpVRQKd8eJ9YpoO4cSXIwH3yt1hKUuvdlKvrfY=; b=Sys/oQTn+H+DGa3+q3SbuCyR9H3RRGf3Vv6iDyJ3icFekHyb7zQ4ptFi2A9fUYTOtV fOM+5/dEm3UVeJgjFHOAM/WTilmwwLeh4zKH8RVoLM8b3qU3uJbinCa1phfVNc+0H/qr a2jAq6b5hCcuSQCWAkwhpFuV8tyIZKdn8xYPE+JRW29aeg0QU//Ldj+MbmPmrHl4yNWh bGj6xLd1D6XEQstlvFDo8fOXYVMIBsxgbJKulHDu1olZp4cPKRrDMSpwISeowBFCNXpU ZLS/j9q6DlB3icHpTQKHvUt+F9s5ft+nLcrPqL2PW1QPD6fwyZ3FEHZVe/jvokh5y06H 6BGg== X-Gm-Message-State: AOAM532FZd3UrttHmCzZod/5nXBsIx9BcmiUb+E32tZ6bwQXVaF5onYQ YtJJ898K4QWZxI9jyJFB2euHF59YfCmiNLq4 X-Received: by 2002:aca:fdc7:: with SMTP id b190mr317029oii.14.1619047969412; Wed, 21 Apr 2021 16:32:49 -0700 (PDT) Received: from sequoia (162-237-133-238.lightspeed.rcsntx.sbcglobal.net. [162.237.133.238]) by smtp.gmail.com with ESMTPSA id t19sm229596otm.40.2021.04.21.16.32.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Apr 2021 16:32:49 -0700 (PDT) Date: Wed, 21 Apr 2021 18:32:39 -0500 From: Tyler Hicks To: Al Viro Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Aditya Pakki , Michael Halcrow Subject: Re: [PATCH 053/190] Revert "ecryptfs: replace BUG_ON with error handling code" Message-ID: <20210421233239.GA177816@sequoia> References: <20210421130105.1226686-1-gregkh@linuxfoundation.org> <20210421130105.1226686-54-gregkh@linuxfoundation.org> <20210421161329.GD4991@sequoia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-04-21 17:03:24, Al Viro wrote: > On Wed, Apr 21, 2021 at 11:13:29AM -0500, Tyler Hicks wrote: > > > > It *is* functionally harmless, AFAICS, but only because the condition > > > is really impossible. However, > > > * it refers to vague (s)tool they'd produced, nevermind that > > > all they really do is "find BUG_ON(), replace with returning an error". > > > * unlike BUG_ON(), the replacement does *NOT* document the > > > fact that condition should be impossible. > > > IMO either should be sufficient for rejecting the patch. > > > > I agree that it was not a malicious change. There are other places > > within the same function that return -EINVAL and the expectation is that > > errors from this function should be handled safely. > > Umm... Assuming that failure exits in the callers will function properly > if those conditions are true. Which is not obvious at all. > > > That said, I can find no real-world reports of this BUG_ON() ever being > > a problem and I don't think that there's any actual need for this > > change. So, I'm alright with it being reverted considering the > > circumstances. > > AFAICS, at least some parts of that BUG_ON() are provably impossible > (e.g. NULL crypt_stat would've oopsed well upstream of the only call > of that function). ECRYPTFS_STRUCT_INITIALIZED is set after > ecryptfs_alloc_inode() and never cleared, i.e. it should be present > in ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat.flags for > all inodes. And crypt_stat we are passing to that thing is > calculated as &(ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat), > which is another reason why it can't be NULL. I agree. > > Incidentally, what's ecryptfs_setattr() doing with similar check? > It had been introduced in e10f281bca03 "eCryptfs: initialize crypt_stat > in setattr", which claims > Recent changes in eCryptfs have made it possible to get to ecryptfs_setattr() > with an uninitialized crypt_stat struct. This results in a wide and colorful > variety of unpleasantries. This patch properly initializes the crypt_stat > structure in ecryptfs_setattr() when it is necessary to do so. > and AFAICS at that point the call of ecryptfs_init_crypt_stat() in > ecryptfs_alloc_inode() had already been there and EXCRYPTFS_STRUCT_INITIALIZED > had been (unconditionally) set by it. So how could that check trigger in > ecryptfs_setattr()? No direct calls of that function (then as well as now), > it's only reachable as ecryptfs_{symlink,dir,main}_iops.setattr. The first > two could only end up set by ecryptfs_interpose(), for inode returned by > iget5_locked() (i.e. one that had been returned by ->alloc_inode()), > the last is set by ecryptfs_init_inode(), called by ecryptfs_inode_set(), > passed as callback to iget5_locked() by the same ecryptfs_interpose(). > IOW, again, the inode must have been returned by ->alloc_inode(). > > I realize that it had been a long time ago, but... could somebody > recall what that patch had been about? Michael? I looked through the commits that proceeded e10f281bca03 and the only thing I can think of is the addition of "passthrough" mode where the lower, encrypted data can be directly read from the eCryptfs mount. It was introduced in commit e77a56ddceee ("[PATCH] eCryptfs: Encrypted passthrough"), several months before e10f281bca03. However, I don't see how it would have left us with an uninitialized crypt_stat in setattr. Tyler > Commit in question contains another (and much bigger) chunk; do > the comments in commit message refer to it? Because it really > looks like > if (!(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED)) > ecryptfs_init_crypt_stat(crypt_stat); > part in ecryptfs_setattr() is a confusing no-op...