Re: [RFC PATCH v2] fs/xattr: add *at family syscalls
On Mon, May 15, 2023 at 4:52 PM Christian Brauner wrote:
>
> On Mon, May 15, 2023 at 04:04:21PM +0300, Amir Goldstein wrote:
> > On Mon, May 15, 2023 at 1:33 PM Christian Brauner
> > wrote:
> > >
> > > On Thu, May 11, 2023 at 05:08:02PM +0200, Christian Göttsche wrote:
> > > > Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> > > > removexattrat(). Those can be used to operate on extended attributes,
> > > > especially security related ones, either relative to a pinned directory
> > > > or on a file descriptor without read access, avoiding a
> > > > /proc//fd/ detour, requiring a mounted procfs.
> > > >
> > > > One use case will be setfiles(8) setting SELinux file contexts
> > > > ("security.selinux") without race conditions.
> > > >
> > > > Add XATTR flags to the private namespace of AT_* flags.
> > > >
> > > > Use the do_{name}at() pattern from fs/open.c.
> > > >
> > > > Use a single flag parameter for extended attribute flags (currently
> > > > XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> > > > syscall arguments in setxattrat().
> > > >
> > > > Previous approach ("f*xattr: allow O_PATH descriptors"):
> > > > https://lore.kernel.org/all/20220607153139.35588-1-cgzo...@googlemail.com/
> > > > v1 discussion:
> > > > https://lore.kernel.org/all/20220830152858.14866-2-cgzo...@googlemail.com/
> > > >
> > > > Signed-off-by: Christian Göttsche
> > > > CC: x...@kernel.org
> > > > CC: linux-al...@vger.kernel.org
> > > > CC: linux-ker...@vger.kernel.org
> > > > CC: linux-arm-ker...@lists.infradead.org
> > > > CC: linux-i...@vger.kernel.org
> > > > CC: linux-m...@lists.linux-m68k.org
> > > > CC: linux-m...@vger.kernel.org
> > > > CC: linux-par...@vger.kernel.org
> > > > CC: linuxppc-dev@lists.ozlabs.org
> > > > CC: linux-s...@vger.kernel.org
> > > > CC: linux...@vger.kernel.org
> > > > CC: sparcli...@vger.kernel.org
> > > > CC: linux-fsde...@vger.kernel.org
> > > > CC: au...@vger.kernel.org
> > > > CC: linux-a...@vger.kernel.org
> > > > CC: linux-...@vger.kernel.org
> > > > CC: linux-security-mod...@vger.kernel.org
> > > > CC: seli...@vger.kernel.org
> > > > ---
> > >
> > > Fwiw, your header doesn't let me see who the mail was directly sent to
> > > so I'm only able to reply to lists which is a bit pointless...
> > >
> > > > v2:
> > > > - squash syscall introduction and wire up commits
> > > > - add AT_XATTR_CREATE and AT_XATTR_REPLACE constants
> > >
> > > > +#define AT_XATTR_CREATE 0x1 /* setxattrat(2): set
> > > > value, fail if attr already exists */
> > > > +#define AT_XATTR_REPLACE 0x2 /* setxattrat(2): set value, fail
> > > > if attr does not exist */
> > >
> > > We really shouldn't waste any AT_* flags for this. Otherwise we'll run
> > > out of them rather quickly. Two weeks ago we added another AT_* flag
> > > which is up for merging for v6.5 iirc and I've glimpsed another AT_*
> > > flag proposal in one of the talks at last weeks Vancouver conference
> > > extravaganza.
> > >
> > > Even if we reuse 0x200 for AT_XATTR_CREATE (like we did for AT_EACCESS
> > > and AT_REMOVEDIR) we still need another bit for AT_XATTR_REPLACE.
> > >
> > > Plus, this is really ugly since AT_XATTR_{CREATE,REPLACE} really isn't
> > > in any way related to lookup and we're mixing it in with lookup
> > > modifying flags.
> > >
> > > So my proposal for {g,s}etxattrat() would be:
> > >
> > > struct xattr_args {
> > > __aligned_u64 value;
> > > __u32 size;
> > > __u32 cmd;
> > > };
> > >
> > > So everything's nicely 64bit aligned in the struct. Use the @cmd member
> > > to set either XATTR_REPLACE or XATTR_CREATE and treat it as a proper
> > > enum and not as a flag argument like the old calls did.
> > >
> > > So then we'd have:
> > >
> > > setxattrat(int dfd, const char *path, const char __user *name,
> > >struct xattr_args __user *args, size_t size, unsigned int
> > > flags)
> > > getxattrat(int dfd, const char *path, const char __user *name,
> > >struct xattr_args __user *args, size_t size, unsigned int
> > > flags)
> > >
> > > The current in-kernel struct xattr_ctx would be renamed to struct
> > > kernel_xattr_args and then we do the usual copy_struct_from_user()
> > > dance:
> > >
> > > struct xattr_args args;
> > > err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
> > >
> > > and then go on to handle value/size for setxattrat()/getxattrat()
> > > accordingly.
> > >
> > > getxattr()/setxattr() aren't meaningfully filterable by seccomp already
> > > so there's not point in not using a struct.
> > >
> > > If that isn't very appealing then another option is to add a new flag
> > > namespace just for setxattrat() similar to fspick() and move_mount()
> > > duplicating the needed lookup modifying flags.
> > > Thoughts?
> >
> > Here is a thought: I am not sure if I am sorry we did not discuss this API
> > issue in LSFMM or happy that we did not waste our time on this... :-/
> >
> > I must say th
Re: [RFC PATCH v2] fs/xattr: add *at family syscalls
On Mon, May 15, 2023 at 1:33 PM Christian Brauner wrote:
>
> On Thu, May 11, 2023 at 05:08:02PM +0200, Christian Göttsche wrote:
> > Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> > removexattrat(). Those can be used to operate on extended attributes,
> > especially security related ones, either relative to a pinned directory
> > or on a file descriptor without read access, avoiding a
> > /proc//fd/ detour, requiring a mounted procfs.
> >
> > One use case will be setfiles(8) setting SELinux file contexts
> > ("security.selinux") without race conditions.
> >
> > Add XATTR flags to the private namespace of AT_* flags.
> >
> > Use the do_{name}at() pattern from fs/open.c.
> >
> > Use a single flag parameter for extended attribute flags (currently
> > XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> > syscall arguments in setxattrat().
> >
> > Previous approach ("f*xattr: allow O_PATH descriptors"):
> > https://lore.kernel.org/all/20220607153139.35588-1-cgzo...@googlemail.com/
> > v1 discussion:
> > https://lore.kernel.org/all/20220830152858.14866-2-cgzo...@googlemail.com/
> >
> > Signed-off-by: Christian Göttsche
> > CC: x...@kernel.org
> > CC: linux-al...@vger.kernel.org
> > CC: linux-ker...@vger.kernel.org
> > CC: linux-arm-ker...@lists.infradead.org
> > CC: linux-i...@vger.kernel.org
> > CC: linux-m...@lists.linux-m68k.org
> > CC: linux-m...@vger.kernel.org
> > CC: linux-par...@vger.kernel.org
> > CC: linuxppc-dev@lists.ozlabs.org
> > CC: linux-s...@vger.kernel.org
> > CC: linux...@vger.kernel.org
> > CC: sparcli...@vger.kernel.org
> > CC: linux-fsde...@vger.kernel.org
> > CC: au...@vger.kernel.org
> > CC: linux-a...@vger.kernel.org
> > CC: linux-...@vger.kernel.org
> > CC: linux-security-mod...@vger.kernel.org
> > CC: seli...@vger.kernel.org
> > ---
>
> Fwiw, your header doesn't let me see who the mail was directly sent to
> so I'm only able to reply to lists which is a bit pointless...
>
> > v2:
> > - squash syscall introduction and wire up commits
> > - add AT_XATTR_CREATE and AT_XATTR_REPLACE constants
>
> > +#define AT_XATTR_CREATE 0x1 /* setxattrat(2): set value,
> > fail if attr already exists */
> > +#define AT_XATTR_REPLACE 0x2 /* setxattrat(2): set value, fail if
> > attr does not exist */
>
> We really shouldn't waste any AT_* flags for this. Otherwise we'll run
> out of them rather quickly. Two weeks ago we added another AT_* flag
> which is up for merging for v6.5 iirc and I've glimpsed another AT_*
> flag proposal in one of the talks at last weeks Vancouver conference
> extravaganza.
>
> Even if we reuse 0x200 for AT_XATTR_CREATE (like we did for AT_EACCESS
> and AT_REMOVEDIR) we still need another bit for AT_XATTR_REPLACE.
>
> Plus, this is really ugly since AT_XATTR_{CREATE,REPLACE} really isn't
> in any way related to lookup and we're mixing it in with lookup
> modifying flags.
>
> So my proposal for {g,s}etxattrat() would be:
>
> struct xattr_args {
> __aligned_u64 value;
> __u32 size;
> __u32 cmd;
> };
>
> So everything's nicely 64bit aligned in the struct. Use the @cmd member
> to set either XATTR_REPLACE or XATTR_CREATE and treat it as a proper
> enum and not as a flag argument like the old calls did.
>
> So then we'd have:
>
> setxattrat(int dfd, const char *path, const char __user *name,
>struct xattr_args __user *args, size_t size, unsigned int flags)
> getxattrat(int dfd, const char *path, const char __user *name,
>struct xattr_args __user *args, size_t size, unsigned int flags)
>
> The current in-kernel struct xattr_ctx would be renamed to struct
> kernel_xattr_args and then we do the usual copy_struct_from_user()
> dance:
>
> struct xattr_args args;
> err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
>
> and then go on to handle value/size for setxattrat()/getxattrat()
> accordingly.
>
> getxattr()/setxattr() aren't meaningfully filterable by seccomp already
> so there's not point in not using a struct.
>
> If that isn't very appealing then another option is to add a new flag
> namespace just for setxattrat() similar to fspick() and move_mount()
> duplicating the needed lookup modifying flags.
> Thoughts?
Here is a thought: I am not sure if I am sorry we did not discuss this API
issue in LSFMM or happy that we did not waste our time on this... :-/
I must say that I dislike redefined flag namespace like FSPICK_*
just as much as I dislike overloading the AT_* namespace and TBH,
I am not crazy about avoiding this problem with xattr_args either.
A more sane solution IMO could have been:
- Use lower word of flags for generic AT_ flags
- Use the upper word of flags for syscall specific flags
So if it were up to me, I would vote starting this practice:
+ /* Start of syscall specific range */
+ #define AT_XATTR_CREATE 0x1 /* setxattrat(2): set
value, fail if attr already exists */
+ #define AT_XATTR_REPLACE 0x2 /* setxa
Re: [RFC PATCH v2] fs/xattr: add *at family syscalls
On Mon, May 15, 2023 at 04:04:21PM +0300, Amir Goldstein wrote:
> On Mon, May 15, 2023 at 1:33 PM Christian Brauner wrote:
> >
> > On Thu, May 11, 2023 at 05:08:02PM +0200, Christian Göttsche wrote:
> > > Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> > > removexattrat(). Those can be used to operate on extended attributes,
> > > especially security related ones, either relative to a pinned directory
> > > or on a file descriptor without read access, avoiding a
> > > /proc//fd/ detour, requiring a mounted procfs.
> > >
> > > One use case will be setfiles(8) setting SELinux file contexts
> > > ("security.selinux") without race conditions.
> > >
> > > Add XATTR flags to the private namespace of AT_* flags.
> > >
> > > Use the do_{name}at() pattern from fs/open.c.
> > >
> > > Use a single flag parameter for extended attribute flags (currently
> > > XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> > > syscall arguments in setxattrat().
> > >
> > > Previous approach ("f*xattr: allow O_PATH descriptors"):
> > > https://lore.kernel.org/all/20220607153139.35588-1-cgzo...@googlemail.com/
> > > v1 discussion:
> > > https://lore.kernel.org/all/20220830152858.14866-2-cgzo...@googlemail.com/
> > >
> > > Signed-off-by: Christian Göttsche
> > > CC: x...@kernel.org
> > > CC: linux-al...@vger.kernel.org
> > > CC: linux-ker...@vger.kernel.org
> > > CC: linux-arm-ker...@lists.infradead.org
> > > CC: linux-i...@vger.kernel.org
> > > CC: linux-m...@lists.linux-m68k.org
> > > CC: linux-m...@vger.kernel.org
> > > CC: linux-par...@vger.kernel.org
> > > CC: linuxppc-dev@lists.ozlabs.org
> > > CC: linux-s...@vger.kernel.org
> > > CC: linux...@vger.kernel.org
> > > CC: sparcli...@vger.kernel.org
> > > CC: linux-fsde...@vger.kernel.org
> > > CC: au...@vger.kernel.org
> > > CC: linux-a...@vger.kernel.org
> > > CC: linux-...@vger.kernel.org
> > > CC: linux-security-mod...@vger.kernel.org
> > > CC: seli...@vger.kernel.org
> > > ---
> >
> > Fwiw, your header doesn't let me see who the mail was directly sent to
> > so I'm only able to reply to lists which is a bit pointless...
> >
> > > v2:
> > > - squash syscall introduction and wire up commits
> > > - add AT_XATTR_CREATE and AT_XATTR_REPLACE constants
> >
> > > +#define AT_XATTR_CREATE 0x1 /* setxattrat(2): set
> > > value, fail if attr already exists */
> > > +#define AT_XATTR_REPLACE 0x2 /* setxattrat(2): set value, fail
> > > if attr does not exist */
> >
> > We really shouldn't waste any AT_* flags for this. Otherwise we'll run
> > out of them rather quickly. Two weeks ago we added another AT_* flag
> > which is up for merging for v6.5 iirc and I've glimpsed another AT_*
> > flag proposal in one of the talks at last weeks Vancouver conference
> > extravaganza.
> >
> > Even if we reuse 0x200 for AT_XATTR_CREATE (like we did for AT_EACCESS
> > and AT_REMOVEDIR) we still need another bit for AT_XATTR_REPLACE.
> >
> > Plus, this is really ugly since AT_XATTR_{CREATE,REPLACE} really isn't
> > in any way related to lookup and we're mixing it in with lookup
> > modifying flags.
> >
> > So my proposal for {g,s}etxattrat() would be:
> >
> > struct xattr_args {
> > __aligned_u64 value;
> > __u32 size;
> > __u32 cmd;
> > };
> >
> > So everything's nicely 64bit aligned in the struct. Use the @cmd member
> > to set either XATTR_REPLACE or XATTR_CREATE and treat it as a proper
> > enum and not as a flag argument like the old calls did.
> >
> > So then we'd have:
> >
> > setxattrat(int dfd, const char *path, const char __user *name,
> >struct xattr_args __user *args, size_t size, unsigned int flags)
> > getxattrat(int dfd, const char *path, const char __user *name,
> >struct xattr_args __user *args, size_t size, unsigned int flags)
> >
> > The current in-kernel struct xattr_ctx would be renamed to struct
> > kernel_xattr_args and then we do the usual copy_struct_from_user()
> > dance:
> >
> > struct xattr_args args;
> > err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
> >
> > and then go on to handle value/size for setxattrat()/getxattrat()
> > accordingly.
> >
> > getxattr()/setxattr() aren't meaningfully filterable by seccomp already
> > so there's not point in not using a struct.
> >
> > If that isn't very appealing then another option is to add a new flag
> > namespace just for setxattrat() similar to fspick() and move_mount()
> > duplicating the needed lookup modifying flags.
> > Thoughts?
>
> Here is a thought: I am not sure if I am sorry we did not discuss this API
> issue in LSFMM or happy that we did not waste our time on this... :-/
>
> I must say that I dislike redefined flag namespace like FSPICK_*
> just as much as I dislike overloading the AT_* namespace and TBH,
> I am not crazy about avoiding this problem with xattr_args either.
>
> A more sane solution IMO could have been:
> - Use lower word of flags for generic AT_ flags
> - Use the upper
Re: [RFC PATCH v2] fs/xattr: add *at family syscalls
On Thu, May 11, 2023 at 05:08:02PM +0200, Christian Göttsche wrote:
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat(). Those can be used to operate on extended attributes,
> especially security related ones, either relative to a pinned directory
> or on a file descriptor without read access, avoiding a
> /proc//fd/ detour, requiring a mounted procfs.
>
> One use case will be setfiles(8) setting SELinux file contexts
> ("security.selinux") without race conditions.
>
> Add XATTR flags to the private namespace of AT_* flags.
>
> Use the do_{name}at() pattern from fs/open.c.
>
> Use a single flag parameter for extended attribute flags (currently
> XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> syscall arguments in setxattrat().
>
> Previous approach ("f*xattr: allow O_PATH descriptors"):
> https://lore.kernel.org/all/20220607153139.35588-1-cgzo...@googlemail.com/
> v1 discussion:
> https://lore.kernel.org/all/20220830152858.14866-2-cgzo...@googlemail.com/
>
> Signed-off-by: Christian Göttsche
> CC: x...@kernel.org
> CC: linux-al...@vger.kernel.org
> CC: linux-ker...@vger.kernel.org
> CC: linux-arm-ker...@lists.infradead.org
> CC: linux-i...@vger.kernel.org
> CC: linux-m...@lists.linux-m68k.org
> CC: linux-m...@vger.kernel.org
> CC: linux-par...@vger.kernel.org
> CC: linuxppc-dev@lists.ozlabs.org
> CC: linux-s...@vger.kernel.org
> CC: linux...@vger.kernel.org
> CC: sparcli...@vger.kernel.org
> CC: linux-fsde...@vger.kernel.org
> CC: au...@vger.kernel.org
> CC: linux-a...@vger.kernel.org
> CC: linux-...@vger.kernel.org
> CC: linux-security-mod...@vger.kernel.org
> CC: seli...@vger.kernel.org
> ---
Fwiw, your header doesn't let me see who the mail was directly sent to
so I'm only able to reply to lists which is a bit pointless...
> v2:
> - squash syscall introduction and wire up commits
> - add AT_XATTR_CREATE and AT_XATTR_REPLACE constants
> +#define AT_XATTR_CREATE 0x1 /* setxattrat(2): set value,
> fail if attr already exists */
> +#define AT_XATTR_REPLACE 0x2 /* setxattrat(2): set value, fail if
> attr does not exist */
We really shouldn't waste any AT_* flags for this. Otherwise we'll run
out of them rather quickly. Two weeks ago we added another AT_* flag
which is up for merging for v6.5 iirc and I've glimpsed another AT_*
flag proposal in one of the talks at last weeks Vancouver conference
extravaganza.
Even if we reuse 0x200 for AT_XATTR_CREATE (like we did for AT_EACCESS
and AT_REMOVEDIR) we still need another bit for AT_XATTR_REPLACE.
Plus, this is really ugly since AT_XATTR_{CREATE,REPLACE} really isn't
in any way related to lookup and we're mixing it in with lookup
modifying flags.
So my proposal for {g,s}etxattrat() would be:
struct xattr_args {
__aligned_u64 value;
__u32 size;
__u32 cmd;
};
So everything's nicely 64bit aligned in the struct. Use the @cmd member
to set either XATTR_REPLACE or XATTR_CREATE and treat it as a proper
enum and not as a flag argument like the old calls did.
So then we'd have:
setxattrat(int dfd, const char *path, const char __user *name,
struct xattr_args __user *args, size_t size, unsigned int flags)
getxattrat(int dfd, const char *path, const char __user *name,
struct xattr_args __user *args, size_t size, unsigned int flags)
The current in-kernel struct xattr_ctx would be renamed to struct
kernel_xattr_args and then we do the usual copy_struct_from_user()
dance:
struct xattr_args args;
err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
and then go on to handle value/size for setxattrat()/getxattrat()
accordingly.
getxattr()/setxattr() aren't meaningfully filterable by seccomp already
so there's not point in not using a struct.
If that isn't very appealing then another option is to add a new flag
namespace just for setxattrat() similar to fspick() and move_mount()
duplicating the needed lookup modifying flags.
Thoughts?
Re: [RFC PATCH v2] fs/xattr: add *at family syscalls
Hi Christian,
On Thu, May 11, 2023 at 5:10 PM Christian Göttsche
wrote:
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat(). Those can be used to operate on extended attributes,
> especially security related ones, either relative to a pinned directory
> or on a file descriptor without read access, avoiding a
> /proc//fd/ detour, requiring a mounted procfs.
>
> One use case will be setfiles(8) setting SELinux file contexts
> ("security.selinux") without race conditions.
>
> Add XATTR flags to the private namespace of AT_* flags.
>
> Use the do_{name}at() pattern from fs/open.c.
>
> Use a single flag parameter for extended attribute flags (currently
> XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> syscall arguments in setxattrat().
>
> Previous approach ("f*xattr: allow O_PATH descriptors"):
> https://lore.kernel.org/all/20220607153139.35588-1-cgzo...@googlemail.com/
> v1 discussion:
> https://lore.kernel.org/all/20220830152858.14866-2-cgzo...@googlemail.com/
>
> Signed-off-by: Christian Göttsche
Thanks for your patch!
The syscall numbers conflict with those used in "[PATCH] cachestat:
wire up cachestat for other architectures", so this needs some
synchronization.
https://lore.kernel.org/linux-sh/20230510195806.2902878-1-npha...@gmail.com
> arch/m68k/kernel/syscalls/syscall.tbl | 4 +
For m68k:
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
