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 5C0ACEB64DA for ; Fri, 14 Jul 2023 12:26:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7666F6B0071; Fri, 14 Jul 2023 08:26:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7165A6B0072; Fri, 14 Jul 2023 08:26:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5DFF56B0074; Fri, 14 Jul 2023 08:26:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 49B526B0071 for ; Fri, 14 Jul 2023 08:26:54 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 104801C7F54 for ; Fri, 14 Jul 2023 12:26:54 +0000 (UTC) X-FDA: 81010141548.27.7722351 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf18.hostedemail.com (Postfix) with ESMTP id 44C5C1C000B for ; Fri, 14 Jul 2023 12:26:52 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="G54vAh/s"; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf18.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=1689337612; 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=BkfQfVikMIrHzTVtUjiMavzqENuo5sD9VSP88qybtDo=; b=qXJ+VUqn0TqzXtizyevKtXpzExVC6XJyYC7BgFrUuTyXE0OtSRNJecnB6cj6tW6RZGlpf0 hV+2LKdlpnrGQvcl03vGLE86ZBdaVeTesoOuEgaH8slKCp+olY3mXRCcVxrU0E48EOn4sx Y+npcEerMKgcRsy31qwy7Z+sPFvGc3o= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="G54vAh/s"; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf18.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=1689337612; a=rsa-sha256; cv=none; b=rbrK1W7N90eq9CQYLyTeEDqYHIjEoyhpV9SM74WjmbSR7+p1cpmrWkQzWhNL91CUErurGL SAHuQD73QfL1ChLfT1GdeTbNOC4y3X75wCIXqbfljcMtz+0tZnwe8K+KxNkxgmbZXMMVoc BZ0lezxzf+WBeERePxJ7MF3Q/t+Fd2E= 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 46F2861CFD; Fri, 14 Jul 2023 12:26:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C7686C433C7; Fri, 14 Jul 2023 12:26:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689337610; bh=xIcj5qUuQUu1ELhgWDH8nrX5vd05Q0RYyk2mpoN6BCg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=G54vAh/sovlSgetpZXD7UBAnYXv4mYRm65EEHuxuIRZZ+n7A1LgETTtu7SkvnAuSk kqSK4tHZQVXcPOJh5qduHSJm6IsZAukzO+lnH4ejLwt+UGOoSZsapfT0m6uqCJQXqR 4qL8dmg9BORNKStP3SmuAffTkv6b3J99zN0O2phQvW/8/L0EqneksdPX+uxvcVXYQ6 oRG4lnE7f1K5pq70R5fd2J9J9tCTzKhcIMsqePgHTOf+uod3RDl71hH1x4fX2PiBjJ nKaaOAOUCaOi21pB66nNmpr0+Dr9wnvCYwLCXjkrU7uTLOEa8Ux/E7zrHyEhwzh5ML hyWNKDPFaoHNw== Date: Fri, 14 Jul 2023 14:26:44 +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: <20230714122644.l6g4wr3jb7fmkf7x@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-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 44C5C1C000B X-Stat-Signature: xmz64dgcuu9aud9j68ymfzg8a1pyaimf X-HE-Tag: 1689337612-700616 X-HE-Meta: U2FsdGVkX1+5GMxHR/GZ3i5esRpZLq9AKf0Uts/KncsV5mVJ+LFMP4zBJ+7GK/mU4TNVOFBGSW6BzJcAP14roEpcsluNbvOGWO4BtHRJUDGc6FG9b1ldaqooEcKM7uslMr3yoLMl8nLBNMlqg32ltkaKjn/L98gqresRDO/KLoNpi+bAOCisHy6UO2hDhWD1Whh72e3JkudO7dMsJXCVv/0ujmq1Z6XeDxNK5mBvPiurtb6PWIbXh2J/UxyT2z/Y9UIf3V94ElQvi1PFtjKtnQOWKiDmS0EszsYVgy9Os8Kqp0wU8DWuXmpbudFIiF5roV708L+dkIGlot2etkZAQ4YqfS4O+pIxsASP60UOrOwx3kW2m1own3lPe4tvSYfFDfadKljPCbaUASET070xuqHyb0jBhNxbr0RHT06wHvL/mqkcmaFW5GHp4m1lGRKL/KKVQdUVv43WxO0QpFEnPhajBOnWH7KQvbJMWbhc3NLaEVJvpndffV1spyGIjd9SS4OkUpencR36jzUqE7vqSPqjKXgeFiB9uSxn7jtAIaKgBrRLkYE3rMu1r/Vb4QGEkWBKwFDFuxBhE5mlzWaPNsC9+WRWJWXaBh46fx0XSzeU8WmtpK5olX9h90OjjmdmNUjfgrCD1SGoLVi6B7wjUsgQ7HRgE4Xi8xiOj0/zLNMCFvaOun6g1792aR5mPHGrZ3swjDTkHgtnOhLhkc6bDi02kh6Rg4Hn1S/HtKU/iEBpoyCXc9HSZLSHC/fl+dQ8ES5+lmvJ0XGyaL+XnmykSHwcVTKT52GpV++uo3fOynszODJnP0aBjsfB54wOWH5fXgj8zkTke4YjYkS+Vbs6c77F4E/z5M1mbqxDTCYpdkhxFS8BzimZDJ4auaNa8WOby/Qe31hKoEBVFYdXMMjN25xUaUQIbF7qlU809cxRKA34zk/oiblSOwro44cv1ebCXbgtPhwDF9nyrOoGR56 Ab9+WVqc mvs3WejNpZZNjHkddwhQZj4KQSdYEUvCEV+2yFOf/EQnM0/egQxEWrf6b0isNx9XksxZT0JnvN12WLMu8oGRIEqH4pksM8LMtGbsfTlwWBHuJGeONStxJYDkIKnc0kWKFkr+HUCmk6UniSOcg+U88bij4qHDpHJbw3oVSE2osz7DwN4c36rWdiZxQHd/EvPcLoLIPrE2qQEshlAraOTg5RStgQDrxhHQ+e/wrvww0nC2Klsh7oYtmUY3xMUlmwtNcPKBOgaB3SVVAQxDhgtqbB+GA6jk9/M47P2Y+ 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: > > > > @@ -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: > > 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. I was just going to rebase these updated changes on top of linux-next, and I realized the patches are already there. Wouldn't it be better if I send a follow-up patch on top of linux-next, applying these changes, as a Fixes: tag? -- Carlos > > 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. > > --- > > 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.