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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 22937C0015E for ; Fri, 14 Jul 2023 10:40:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4331E6B0071; Fri, 14 Jul 2023 06:40:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3BC4F6B0072; Fri, 14 Jul 2023 06:40:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 25D1B6B0074; Fri, 14 Jul 2023 06:40:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 15D586B0071 for ; Fri, 14 Jul 2023 06:40:51 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C9F83C01F4 for ; Fri, 14 Jul 2023 10:40:50 +0000 (UTC) X-FDA: 81009874260.26.794F802 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf11.hostedemail.com (Postfix) with ESMTP id 2075A40011 for ; Fri, 14 Jul 2023 10:40:48 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=jyhjUwyC; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf11.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689331249; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=KzAv/xU9Oeph7aVYBaaD8AJun/xanAZczuKtb4lg0c8=; b=w6wVECBXkfb3CV6C2JyYqIs3oeU0IfNnXNQ/qjQge6pIPqn3K/dGihXg57cAEVWG+xTnMG Plgrk3fQeGhx24QO4uolXo/gvdSlMF5DBwFrZ6KgEExvXn+fSdOM4uWglmkSZos1m1B9Rj 0FBUvg//PL/o1l1LJxWbEqGuk6LJXAw= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=jyhjUwyC; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf11.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689331249; a=rsa-sha256; cv=none; b=SIgWPJM9lC7YyVM0rBwTCpoBqHNFO3Gi548XutIytA4Hrn0xSeGHGnAJGdLgjLvmNYl8QU FcCS4nRNmgjwz1n/fj7AjxZWwVh/S4HzsyNblxBeQDomkiSxd95tgGxsfdAjEzwhAXiQGc kCeIwQJNfKsn4mV/kuNGjXvgbiipEfE= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 3B92B61CDF; Fri, 14 Jul 2023 10:40:48 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2EDA1C433C7; Fri, 14 Jul 2023 10:40:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689331247; bh=McAF6OOvBTRjliiSqjAaea2L7Hppjte3npPE1j55d9Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jyhjUwyCzAGjLkYvRdPlRoaD0yDjf02kOoGB/IUtc2pBMnFj05McGDmEgmlnsI+v1 8ZJvYz3/1YMhtnZ48RV2LiqJFPKEPzmEXtShw4e5ln1HeljyMGpiRceBIvnZz6m2y4 EkNORX06d75v+FKFoYwGYqwzthnGtLxZpH6Kpn2kRQE11l4Xb3Dk08pQb986lHaxNi ywDeWcCFt/7BskAwjhPuBmkZ9U3w56FUlRKmthprRmUYjARJvJfMRPkwpF+q2Hq7Mz ukv+hc/d2+OYCgHf5wsvwkrIwzqPIp6o/y/lCjOo/VOn7IU16LFmznuZXB1O9/idmk Cg/aCBGTRD9/A== Date: Fri, 14 Jul 2023 12:40:42 +0200 From: Carlos Maiolino To: Christian Brauner Cc: linux-fsdevel@vger.kernel.org, jack@suse.cz, akpm@linux-foundation.org, viro@zeniv.linux.org.uk, linux-mm@kvack.org, djwong@kernel.org, hughd@google.com, mcgrof@kernel.org Subject: Re: [PATCH 5/6] shmem: quota support Message-ID: <20230714104042.ooih4eqd2t7wjkoc@andromeda> References: <20230713134848.249779-1-cem@kernel.org> <20230713134848.249779-6-cem@kernel.org> <20230714-messtechnik-knieprobleme-5d0a3abb4413@brauner> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230714-messtechnik-knieprobleme-5d0a3abb4413@brauner> X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 2075A40011 X-Stat-Signature: qm4itmd1tdxgyry7139e7nqsxpm57ykn X-Rspam-User: X-HE-Tag: 1689331248-924747 X-HE-Meta: U2FsdGVkX19V9TjKFuckakb7mvQrfGReSQ3SR67MqFX75KqHPv/EZ+8cnrfrH7zzb/D3ojoSjzTJbDsf5Th7e+63MPzkoOnNnLcT4r07jzWdejG63CQiKe77vVp/l2xnlSpgsRfBTAIC2b7is+x4HU8iI8HK3oJ+2woigBNzCSwj+zO8WFVwdLdg/lECfICwgcoAsIke9Dy3+7y+LpXKzw6gYLuXtAdiY13oNJBObiC3WEFqBLm6m28P4K99KJfIUkBcIgAEzUT2aT4PTWUFnfoczQKsrnCQIW6liqq0FolsHb+fOEWAzLElQtbMglxFlY1YvjFFh2G55AaPSiYqdWndZqWS+QPycbghycS91QcfDlByUzo/LIwNf6r/WUojZ52HQS5A3HtXMtlCs2ok6ka04/+1yMqyY74yBjUhtJQOhjHKeRWNOOIf61NG5iZ2muTfozm/vowG9bZ8bcG/Yfa5jzM3gXGKNmXzPnfgIktnBQA6JFRpOwINTorC7nnIDXGlzrRHdGRxL1wqi8Te1gK0N6Y/oIGDEyOGDUGgrPHy2NcQNCc3tGQv1IXpYkP4w5rRCMSnwYB5qpJsfYGw3mv0j4JK/zQ2fKSLFJgATBVa35kpAMTtDE1V9Ai4U12oAhdxVWJnWaUbxW79/vQCHnA90G7/fjvEu+yCf8HZw9/dzLAKc3G4ZLhXynRzjAJInVH9k2Dw0HCU5MCRMEE9lFQNCT6NjbZ/BZakeFlBFT8f05Dvopu0Ifntmwv9Vy36M/7l5edLfwM1UbZt8x91wxQ6avRJZ6pQ6+YR9aetvDqRqrNoXzLdTWj/Ce45G3cWJwioNq+mBNAkAU7iQseojPmmIbLcRG3KSvM1/vE/HuXPJ9uWKMzDM7cPEid0hspDRUeTKwkQevRRqpnDFq6i0nZzI99qBJSiBASzO04pKuFOwztIIZqteN8srD9aLB1Ca0Hqq0mZW30imPr/lkm TDseUQ/g 0cdH45ZC7v1RLjHDMx2R5Uv+oN0GKNkkZm09RrnaMs7QwIgobousFc3wBdkqBo4xWO85R4iIZ85W1FhI0MIWUndQjIW8Rq4gkj6r8Z6ueIQK93ILM2HVl3fnKPaRMDb8N6mlY9XPAW2hlaySeosU78tvftmcvPvPHDA/mNXC3JFShzpkyGgGYxTQiDskn6eN+798BaUlDJoHJbApULwQnPy4AJMEFDZjs87vEQQ7uR9BquZk72ai/3D/mXPWQFV2DYvgZlXuGri3R5NdrFZN8krCn6cgQt3C8hBII X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi Christian. > > @@ -3736,6 +3853,18 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param) > > ctx->noswap = true; > > ctx->seen |= SHMEM_SEEN_NOSWAP; > > break; > > + case Opt_quota: > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > + ctx->quota_types |= (QTYPE_MASK_USR | QTYPE_MASK_GRP); > > + break; > > + case Opt_usrquota: > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > + ctx->quota_types |= QTYPE_MASK_USR; > > + break; > > + case Opt_grpquota: > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > + ctx->quota_types |= QTYPE_MASK_GRP; > > + break; > > } > > return 0; > > I mentioned this in an earlier review; following the sequence: Ok, my apologies, I should have lost it in the noise. > > if (ctx->seen & SHMEM_SEEN_QUOTA) > -> shmem_enable_quotas() > -> dquot_load_quota_sb() > > to then figure out that in dquot_load_quota_sb() we fail if > sb->s_user_ns != &init_user_ns is too subtle for a filesystem that's > mountable by unprivileged users. Every few months someone will end up > stumbling upon this code and wonder where it's blocked. There isn't even > a comment in the code. > > Aside from that it's also really unfriendly to users because they may go > through setting up a tmpfs instances in the following way: > > fd_fs = fsopen("tmpfs"); > > User now enables quota: > > fsconfig(fd_fs, ..., "quota", ...) = 0 > > and goes on to set a bunch of other options: > > fsconfig(fd_fs, ..., "inode64", ...) = 0 > fsconfig(fd_fs, ..., "nr_inodes", ...) = 0 > fsconfig(fd_fs, ..., "nr_blocks", ...) = 0 > fsconfig(fd_fs, ..., "huge", ...) = 0 > fsconfig(fd_fs, ..., "mode", ...) = 0 > fsconfig(fd_fs, ..., "gid", ...) = 0 > > everything seems dandy and they create the superblock: > > fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...) = -EINVAL > > which fails. > > The user has not just performed 9 useless system calls they also have > zero clue what mount option caused the failure. > > What this code really really should do is fail at: > > fsconfig(fd_fs, ..., "quota", ...) = -EINVAL > > and log an error that the user can retrieve from the fs context. IOW, > > diff --git a/mm/shmem.c b/mm/shmem.c > index 083ce6b478e7..baca8bf44569 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -3863,14 +3863,20 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param) > ctx->seen |= SHMEM_SEEN_NOSWAP; > break; > case Opt_quota: > + if (fc->user_ns != &init_user_ns) > + return invalfc(fc, "Quotas in unprivileged tmpfs mounts unsupported"); > ctx->seen |= SHMEM_SEEN_QUOTA; > ctx->quota_types |= (QTYPE_MASK_USR | QTYPE_MASK_GRP); > break; > case Opt_usrquota: > + if (fc->user_ns != &init_user_ns) > + return invalfc(fc, "Quotas in unprivileged tmpfs mounts unsupported"); > ctx->seen |= SHMEM_SEEN_QUOTA; > ctx->quota_types |= QTYPE_MASK_USR; > break; > case Opt_grpquota: > + if (fc->user_ns != &init_user_ns) > + return invalfc(fc, "Quotas in unprivileged tmpfs mounts unsupported"); > ctx->seen |= SHMEM_SEEN_QUOTA; > ctx->quota_types |= QTYPE_MASK_GRP; > break; > > This exactly what we already to for the "noswap" option btw. > > Could you fold these changes into the patch and resend, please? > I synced with Andrew earlier and I'll be taking this series. Thanks! I will sure do it, I'll update the patch, build, test and send it again in a few minutes. > > --- > > And btw, the *_SEEN_* logic for mount options is broken - but that's not > specific to your patch. Imagine: > > fd_fs = fsopen("tmpfs"); > fsconfig(fd_fs, ..., "nr_inodes", 0, "1000") = 0 > > Now ctx->inodes == 1000 and ctx->seen |= SHMEM_SEEN_INODES. > > Now the user does: > > fsconfig(fd_fs, ..., "nr_inodes", 0, "-1234") = -EINVAL > > This fails, but: > > ctx->inodes = memparse(param->string, &rest); > if (*rest) > goto bad_value; > > will set ctx->inodes to whatever memparse returns but leaves > SHMEM_SEEN_INODES raised in ctx->seen. Now superblock creation may > succeed with a garbage inode limit. This should affect other mount > options as well. Interesting. Thanks for the heads up. I'll look in more details into it when I start working for namespace support for quotas (as we spoke previously). Cheers. -- Carlos