From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 528C71DF733; Sun, 2 Mar 2025 12:20:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740918022; cv=none; b=n206KnPNuxRAyMKpgGdLPE0WPGXvscM1+a5SvBOaEvOg7RSZXcIiBj5KMjVQO23NuJMvx6F15jJCSuM1YBk2SfBKTToh82BRyfpZxpqtv6G/zxgCH7SLC8VViZAHTOFS1uhBjANMa+ivTcUX8Xy6WVt6WywGtkQDTR21yw9wkPE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740918022; c=relaxed/simple; bh=g6N4TpIZBNJ6Fua7mVfbapNWidlbTbgUQQkGOYDnX5w=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=OhIV7TsKKFc9eF8Xcj3iWbZUuqyXeuRLYI4fltL40sYCL0zmNvZZgSrW/LcmjLP49PwgXOSAiNPdn8itlL9KSdyhhRAb6TYOuL7xfyZv4VGAcYkrdTDs9GrM9S0livIwI86tzlEHITh8WM5GzjYMMSDzwbwJyMqQzhEkSByttv0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=XLAcy+qL; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="XLAcy+qL" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 69891C4CED6; Sun, 2 Mar 2025 12:20:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1740918021; bh=g6N4TpIZBNJ6Fua7mVfbapNWidlbTbgUQQkGOYDnX5w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XLAcy+qLxtnm5pMuivx8y1wbx2whPxwRO/rtRwHb3XGTBqVLv3H+H+3nfqpeiVTcs Z8lfoi1AD8UU6wierd4zqGyjqm2XgDBTFpyi1W0HUpAccz8CZZfwoap8XIs48kX6CM f65c3R0PoNN+S8rIzp2IT3MHFOrvriGPG50wn0FSzq7G1E9xIH+fomhG3PH0clazK/ cuQYFYDwhpmUVesZZFvWCIXQxzNrG6is9mG/+Ao0tNiCJG7f9Ytj2Gp9KWDRZSVXNh idBB961t1iIPDDhfJ52Qjsy9qgiCACyIbivPMIRAKk58il9vU7ldji9EtwFN/hvYOm zwPxA25FKE5uQ== Received: by pali.im (Postfix) id A4C237B3; Sun, 2 Mar 2025 13:20:07 +0100 (CET) Date: Sun, 2 Mar 2025 13:20:07 +0100 From: Pali =?utf-8?B?Um9ow6Fy?= To: Andrey Albershteyn Cc: Amir Goldstein , "Darrick J. Wong" , Richard Henderson , Matt Turner , Russell King , Catalin Marinas , Will Deacon , Geert Uytterhoeven , Michal Simek , Thomas Bogendoerfer , "James E.J. Bottomley" , Helge Deller , Madhavan Srinivasan , Michael Ellerman , Nicholas Piggin , Christophe Leroy , Naveen N Rao , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Yoshinori Sato , Rich Felker , John Paul Adrian Glaubitz , "David S. Miller" , Andreas Larsson , Andy Lutomirski , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , Chris Zankel , Max Filippov , Alexander Viro , Christian Brauner , Jan Kara , =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= , =?utf-8?Q?G=C3=BCnther?= Noack , Arnd Bergmann , linux-alpha@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-m68k@lists.linux-m68k.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-security-module@vger.kernel.org, linux-api@vger.kernel.org, linux-arch@vger.kernel.org, linux-xfs@vger.kernel.org, Theodore Tso Subject: Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls Message-ID: <20250302122007.4oxtugidf4vxx3vy@pali> References: <20250211-xattrat-syscall-v3-1-a07d15f898b2@kernel.org> <20250221181135.GW21808@frogsfrogsfrogs> Precedence: bulk X-Mailing-List: linux-api@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20180716 On Friday 28 February 2025 09:30:38 Andrey Albershteyn wrote: > On 2025-02-21 20:15:24, Amir Goldstein wrote: > > On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong wrote: > > > > > > On Tue, Feb 11, 2025 at 06:22:47PM +0100, Andrey Albershteyn wrote: > > > > From: Andrey Albershteyn > > > > > > > > Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode > > > > extended attributes/flags. The syscalls take parent directory fd and > > > > path to the child together with struct fsxattr. > > > > > > > > This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference > > > > that file don't need to be open as we can reference it with a path > > > > instead of fd. By having this we can manipulated inode extended > > > > attributes not only on regular files but also on special ones. This > > > > is not possible with FS_IOC_FSSETXATTR ioctl as with special files > > > > we can not call ioctl() directly on the filesystem inode using fd. > > > > > > > > This patch adds two new syscalls which allows userspace to get/set > > > > extended inode attributes on special files by using parent directory > > > > and a path - *at() like syscall. > > > > > > > > Also, as vfs_fileattr_set() is now will be called on special files > > > > too, let's forbid any other attributes except projid and nextents > > > > (symlink can have an extent). > > > > > > > > CC: linux-api@vger.kernel.org > > > > CC: linux-fsdevel@vger.kernel.org > > > > CC: linux-xfs@vger.kernel.org > > > > Signed-off-by: Andrey Albershteyn > > > > --- > > > > v1: > > > > https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/ > > > > > > > > Previous discussion: > > > > https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/ > > > > > > > > XFS has project quotas which could be attached to a directory. All > > > > new inodes in these directories inherit project ID set on parent > > > > directory. > > > > > > > > The project is created from userspace by opening and calling > > > > FS_IOC_FSSETXATTR on each inode. This is not possible for special > > > > files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left > > > > with empty project ID. Those inodes then are not shown in the quota > > > > accounting but still exist in the directory. Moreover, in the case > > > > when special files are created in the directory with already > > > > existing project quota, these inode inherit extended attributes. > > > > This than leaves them with these attributes without the possibility > > > > to clear them out. This, in turn, prevents userspace from > > > > re-creating quota project on these existing files. > > > > --- > > > > Changes in v3: > > > > - Remove unnecessary "dfd is dir" check as it checked in user_path_at() > > > > - Remove unnecessary "same filesystem" check > > > > - Use CLASS() instead of directly calling fdget/fdput > > > > - Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org > > > > --- > > > > arch/alpha/kernel/syscalls/syscall.tbl | 2 + > > > > arch/arm/tools/syscall.tbl | 2 + > > > > arch/arm64/tools/syscall_32.tbl | 2 + > > > > arch/m68k/kernel/syscalls/syscall.tbl | 2 + > > > > arch/microblaze/kernel/syscalls/syscall.tbl | 2 + > > > > arch/mips/kernel/syscalls/syscall_n32.tbl | 2 + > > > > arch/mips/kernel/syscalls/syscall_n64.tbl | 2 + > > > > arch/mips/kernel/syscalls/syscall_o32.tbl | 2 + > > > > arch/parisc/kernel/syscalls/syscall.tbl | 2 + > > > > arch/powerpc/kernel/syscalls/syscall.tbl | 2 + > > > > arch/s390/kernel/syscalls/syscall.tbl | 2 + > > > > arch/sh/kernel/syscalls/syscall.tbl | 2 + > > > > arch/sparc/kernel/syscalls/syscall.tbl | 2 + > > > > arch/x86/entry/syscalls/syscall_32.tbl | 2 + > > > > arch/x86/entry/syscalls/syscall_64.tbl | 2 + > > > > arch/xtensa/kernel/syscalls/syscall.tbl | 2 + > > > > fs/inode.c | 75 +++++++++++++++++++++++++++++ > > > > fs/ioctl.c | 16 +++++- > > > > include/linux/fileattr.h | 1 + > > > > include/linux/syscalls.h | 4 ++ > > > > include/uapi/asm-generic/unistd.h | 8 ++- > > > > 21 files changed, 133 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > > diff --git a/fs/inode.c b/fs/inode.c > > > > index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..b2dddd9db4fabaf67a6cbf541a86978b290411ec 100644 > > > > --- a/fs/inode.c > > > > +++ b/fs/inode.c > > > > @@ -23,6 +23,9 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > +#include > > > > +#include > > > > #include > > > > #define CREATE_TRACE_POINTS > > > > #include > > > > @@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap, > > > > return mode & ~S_ISGID; > > > > } > > > > EXPORT_SYMBOL(mode_strip_sgid); > > > > + > > > > +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename, > > > > + struct fsxattr __user *, fsx, unsigned int, at_flags) > > > > > > Should the kernel require userspace to pass the size of the fsx buffer? > > > That way we avoid needing to rev the interface when we decide to grow > > > the structure. > > > > > > > This makes sense to me, but I see that Andreas proposed other ways, > > as long as we have a plan on how to extend the struct if we need more space. > > > > Andrey, I am sorry to bring this up in v3, but I would like to request > > two small changes before merging this API. > > > > This patch by Pali [1] adds fsx_xflags_mask for the filesystem to > > report the supported set of xflags. > > > > It was argued that we can make this change with the existing ioctl, > > because it is not going to break xfs_io -c lsattr/chattr, which is fine, > > but I think that we should merge the fsx_xflags_mask change along > > with getfsxattrat() which is a new UAPI. > > > > The second request is related to setfsxattrat(). > > With current FS_IOC_FSSETXATTR, IIUC, xfs ignores unsupported > > fsx_xflags. I think this needs to be fixed before merging setfsxattrat(). > > It's ok that a program calling FS_IOC_FSSETXATTR will not know > > if unsupported flags will be ignored, because that's the way it is, > > but I think that setfsxattrat() must return -EINVAL for trying to > > set unsupported xflags. > > > > As I explained in [2] I think it is fine if FS_IOC_FSSETXATTR > > will also start returning -EINVAL for unsupported flags, but I would > > like setfsxattrat() to make that a guarantee. > > > > There was an open question, what does fsx_xflags_mask mean > > for setfsxattrat() - it is a mask like in inode_set_flags() as Andreas > > suggested? I think that would be a good idea. > > > > Thanks, > > Amir. > > > > [1] https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/ > > [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjwQJiKAqyjEmKUnq-VihyeSsxyEy2F+J38NXwrAXurFQ@mail.gmail.com/ > > > > I'm fine with making Pali's patchset a dependency for this syscall, > as if vfs_fileattr_set() will start returning EINVAL on unsupported > flags this syscall will pass it through (ioctls will need to ignore > it). And as these syscalls use fsxattr anyway the fsx_xflags_mask > field will be here. > > -- > - Andrey > Hello Andrey, if I understand correctly then it is needed for new setfsxattrat() call to return EINVAL on any unsupported flags since beginning. Then I could extend it for new flags without breaking backward or forward compatibility of the setfsxattrat() call.