| Message ID | cover.1689092120.git.legion@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add a new fchmodat2() syscall | expand |
On Tue, 11 Jul 2023 18:16:02 +0200, Alexey Gladkov wrote: > In glibc, the fchmodat(3) function has a flags argument according to the > POSIX specification [1], but kernel syscalls has no such argument. > Therefore, libc implementations do workarounds using /proc. However, > this requires procfs to be mounted and accessible. > > This patch set adds fchmodat2(), a new syscall. The syscall allows to > pass the AT_SYMLINK_NOFOLLOW flag to disable LOOKUP_FOLLOW. In all other > respects, this syscall is no different from fchmodat(). > > [...] Tools updates usually go separately. Flags argument ported to unsigned int; otherwise unchanged. --- Applied to the master branch of the vfs/vfs.git tree. Patches in the master branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: master [1/5] Non-functional cleanup of a "__user * filename" https://git.kernel.org/vfs/vfs/c/0f05a6af6b7e [2/5] fs: Add fchmodat2() https://git.kernel.org/vfs/vfs/c/8d593559ec09 [3/5] arch: Register fchmodat2, usually as syscall 452 https://git.kernel.org/vfs/vfs/c/2ee63b04f206 [5/5] selftests: Add fchmodat2 selftest https://git.kernel.org/vfs/vfs/c/f175b92081ec
On Tue, Jul 11, 2023 at 06:16:02PM +0200, Alexey Gladkov wrote: > In glibc, the fchmodat(3) function has a flags argument according to the > POSIX specification [1], but kernel syscalls has no such argument. > Therefore, libc implementations do workarounds using /proc. However, > this requires procfs to be mounted and accessible. > > This patch set adds fchmodat2(), a new syscall. The syscall allows to > pass the AT_SYMLINK_NOFOLLOW flag to disable LOOKUP_FOLLOW. In all other > respects, this syscall is no different from fchmodat(). > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html > > Changes since v3 [cover.1689074739.git.legion@kernel.org]: > > * Rebased to master because a new syscall has appeared in master. > * Increased __NR_compat_syscalls as pointed out by Arnd Bergmann. > * Syscall renamed fchmodat4 -> fchmodat2 as suggested by Christian Brauner. > * Returned do_fchmodat4() the original name. We don't need to version > internal functions. > * Fixed warnings found by checkpatch.pl. > > Changes since v2 [20190717012719.5524-1-palmer@sifive.com]: > > * Rebased to master. > * The lookup_flags passed to sys_fchmodat4 as suggested by Al Viro. > * Selftest added. > > Changes since v1 [20190531191204.4044-1-palmer@sifive.com]: > > * All architectures are now supported, which support squashed into a > single patch. > * The do_fchmodat() helper function has been removed, in favor of directly > calling do_fchmodat4(). > * The patches are based on 5.2 instead of 5.1. It's good to see this moving forward. I originally proposed this in a patch submitted in 2020. I suspect implementation details have changed since then, but it might make sense to look back at that discussion if nobody has done so yet (apologies if this was already done and I missed it) to make sure nothing is overlooked -- I remember there were some subtleties with what fs backends might try to do with chmod on symlinks. My proposed commit message also documented a lot of the history of the issue that might be useful to have as context. https://lore.kernel.org/all/20200910170256.GK3265@brightrain.aerifal.cx/T/ Rich
Rather than adding a fchmodat2() syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface to notify_change() in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems. David
* David Howells: > Rather than adding a fchmodat2() syscall, should we add a "set_file_attrs()" > syscall that takes a mask and allows you to set a bunch of stuff all in one > go? Basically, an interface to notify_change() in the kernel that would allow > several stats to be set atomically. This might be of particular interest to > network filesystems. Do you mean atomically as in compare-and-swap (update only if old values match), or just a way to update multiple file attributes with a single system call? Thanks, Florian
On 2023-07-25, David Howells <dhowells@redhat.com> wrote: > Rather than adding a fchmodat2() syscall, should we add a "set_file_attrs()" > syscall that takes a mask and allows you to set a bunch of stuff all in one > go? Basically, an interface to notify_change() in the kernel that would allow > several stats to be set atomically. This might be of particular interest to > network filesystems. Presumably looking something like statx(2) (except hopefully with extensible structs this time :P)? I think that could also be useful, but given this is a fairly straight-forward syscall addition (and it also would resolve the AT_EMPTY_PATH issue for chmod as well as simplify the glibc wrapper), I think it makes sense to take this and we can do set_statx(2) separately?
Florian Weimer <fweimer@redhat.com> wrote: > > Rather than adding a fchmodat2() syscall, should we add a > > "set_file_attrs()" syscall that takes a mask and allows you to set a bunch > > of stuff all in one go? Basically, an interface to notify_change() in the > > kernel that would allow several stats to be set atomically. This might be > > of particular interest to network filesystems. > > Do you mean atomically as in compare-and-swap (update only if old values > match), or just a way to update multiple file attributes with a single > system call? I was thinking more in terms of the latter. AFAIK, there aren't any network filesystems support a CAS interface on file attributes like that. To be able to do a CAS operation, we'd need to pass in the old values as well as the new. Another thing we could look at is doing "create_and_set_attrs()", possibly allowing it to take a list of xattrs also. David
On Tue, Jul 25, 2023 at 07:39:51PM +0100, David Howells wrote: > Florian Weimer <fweimer@redhat.com> wrote: > > > > Rather than adding a fchmodat2() syscall, should we add a > > > "set_file_attrs()" syscall that takes a mask and allows you to set a bunch > > > of stuff all in one go? Basically, an interface to notify_change() in the > > > kernel that would allow several stats to be set atomically. This might be > > > of particular interest to network filesystems. > > > > Do you mean atomically as in compare-and-swap (update only if old values > > match), or just a way to update multiple file attributes with a single > > system call? > > I was thinking more in terms of the latter. AFAIK, there aren't any network > filesystems support a CAS interface on file attributes like that. To be able > to do a CAS operation, we'd need to pass in the old values as well as the new. > > Another thing we could look at is doing "create_and_set_attrs()", possibly > allowing it to take a list of xattrs also. Can we please not let " hey let's invent a new interface to do something that will be hard for underlying filesystems to even provide and that nothing needs because there's no standard API to do it" be the enemy of "fixing a known problem implementing an existing standard API that just requires a simple, clearly-scoped syscall to do it"? Rich
On Tue, Jul 25, 2023 at 07:39:51PM +0100, David Howells wrote: > Florian Weimer <fweimer@redhat.com> wrote: > > > > Rather than adding a fchmodat2() syscall, should we add a > > > "set_file_attrs()" syscall that takes a mask and allows you to set a bunch > > > of stuff all in one go? Basically, an interface to notify_change() in the That system call would likely be blocked in seccomp sandboxes completely as seccomp cannot filter structs. I don't consider this an argument to block new good functionality in general as that would mean arbitrarily limiting us but it is something to keep in mind. If there's additional benefit other than just being able to set mutliple values at once then yeah might be something to discuss. > > > kernel that would allow several stats to be set atomically. This might be > > > of particular interest to network filesystems. > > > > Do you mean atomically as in compare-and-swap (update only if old values > > match), or just a way to update multiple file attributes with a single > > system call? > > I was thinking more in terms of the latter. AFAIK, there aren't any network > filesystems support a CAS interface on file attributes like that. To be able > to do a CAS operation, we'd need to pass in the old values as well as the new. > > Another thing we could look at is doing "create_and_set_attrs()", possibly > allowing it to take a list of xattrs also. That would likely require variable sized pointers in a struct which is something we really try to stay away from. I also think it's not a good idea to lump xattrs toegether with generic file attributes. They should remain a separate api imho.
On Tue, Jul 25, 2023 at 04:58:34PM +0100, David Howells wrote: > Rather than adding a fchmodat2() syscall, should we add a "set_file_attrs()" > syscall that takes a mask and allows you to set a bunch of stuff all in one > go? Basically, an interface to notify_change() in the kernel that would allow > several stats to be set atomically. This might be of particular interest to > network filesystems. > > David > fchmodat2() is a simple addition that fits well with the existing syscalls. It fixes an oversight in fchmodat(). IMO we should just add fchmodat2(), and not get sidetracked by trying to add some super-generalized syscall instead. That can always be done later. - Eric
On Wed, Jul 26, 2023 at 08:57:10PM -0700, Eric Biggers wrote: > On Tue, Jul 25, 2023 at 04:58:34PM +0100, David Howells wrote: > > Rather than adding a fchmodat2() syscall, should we add a "set_file_attrs()" > > syscall that takes a mask and allows you to set a bunch of stuff all in one > > go? Basically, an interface to notify_change() in the kernel that would allow > > several stats to be set atomically. This might be of particular interest to > > network filesystems. > > > > David > > > > fchmodat2() is a simple addition that fits well with the existing syscalls. > It fixes an oversight in fchmodat(). > > IMO we should just add fchmodat2(), and not get sidetracked by trying to add > some super-generalized syscall instead. That can always be done later. Agreed.