* [PATCH] xfs: add selinux labels to whiteout inodes
@ 2022-06-20 22:55 Eric Sandeen
2022-06-20 23:06 ` Eric Sandeen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Sandeen @ 2022-06-20 22:55 UTC (permalink / raw)
To: linux-xfs
We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't
apply an SELinux label on xfs" as it does on other filesystems
(for example, ext4 and tmpfs.) While I'm not quite sure how labels
may interact w/ whiteout files, leaving them as unlabeled seems
inconsistent at best.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
fs/xfs/xfs_inode.c | 14 +++++++++++++-
fs/xfs/xfs_iops.c | 2 +-
fs/xfs/xfs_iops.h | 3 +++
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 52d6f2c..9a43060 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3046,10 +3046,12 @@ struct xfs_iunlink {
static int
xfs_rename_alloc_whiteout(
struct user_namespace *mnt_userns,
+ struct xfs_name *src_name,
struct xfs_inode *dp,
struct xfs_inode **wip)
{
struct xfs_inode *tmpfile;
+ struct qstr name;
int error;
error = xfs_create_tmpfile(mnt_userns, dp, S_IFCHR | WHITEOUT_MODE,
@@ -3057,6 +3059,15 @@ struct xfs_iunlink {
if (error)
return error;
+ name.name = src_name->name;
+ name.len = src_name->len;
+ error = xfs_init_security(VFS_I(tmpfile), VFS_I(dp), &name);
+ if (error) {
+ xfs_finish_inode_setup(tmpfile);
+ xfs_irele(tmpfile);
+ return error;
+ }
+
/*
* Prepare the tmpfile inode as if it were created through the VFS.
* Complete the inode setup and flag it as linkable. nlink is already
@@ -3107,7 +3118,8 @@ struct xfs_iunlink {
* appropriately.
*/
if (flags & RENAME_WHITEOUT) {
- error = xfs_rename_alloc_whiteout(mnt_userns, target_dp, &wip);
+ error = xfs_rename_alloc_whiteout(mnt_userns, src_name,
+ target_dp, &wip);
if (error)
return error;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 29f5b8b8..c7775b7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -76,7 +76,7 @@
* inode, of course, such that log replay can't cause these to be lost).
*/
-STATIC int
+int
xfs_init_security(
struct inode *inode,
struct inode *dir,
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index 2789490..fd92c82 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -17,4 +17,7 @@
int xfs_vn_setattr_size(struct user_namespace *mnt_userns,
struct dentry *dentry, struct iattr *vap);
+int xfs_init_security(struct inode *inode, struct inode *dir,
+ const struct qstr *qstr);
+
#endif /* __XFS_IOPS_H__ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: add selinux labels to whiteout inodes
2022-06-20 22:55 [PATCH] xfs: add selinux labels to whiteout inodes Eric Sandeen
@ 2022-06-20 23:06 ` Eric Sandeen
2022-06-20 23:41 ` Dave Chinner
2022-06-21 1:38 ` [PATCH V2] " Eric Sandeen
2 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2022-06-20 23:06 UTC (permalink / raw)
To: linux-xfs
I had mentioned this problem in passing to dchinner, and came away with
a vague sense that I might be Doing It Wrong. So I should have labeled
the patch RFC, I suppose.
An easy way to demonstrate the selinux label result is with this test
program, as provided by the bug reporter. ls -lZ will then show you
the resulting labels on each file.
#define _GNU_SOURCE
#include <sys/mount.h>
#include <sys/stat.h>
#include <errno.h>
#include <fcntl.h>
#include <sched.h>
#include <stdio.h>
#include <unistd.h>
int main(int argc, char **argv) {
int rc, fd, dirfd;
rc = mkdir("upper", 0700);
if ((rc != 0) && (errno != EEXIST)) {
perror("mkdir");
return rc;
}
rc = unlink("upper/0");
if ((rc != 0) && (errno != ENOENT)) {
perror("unlink");
return rc;
}
rc = unlink("upper/empty");
if ((rc != 0) && (errno != ENOENT)) {
perror("unlink");
return rc;
}
dirfd = open("upper", O_PATH);
if (dirfd == -1) {
perror("open");
return dirfd;
}
fd = creat("upper/empty", 0600);
if (fd == -1) {
perror("creat");
return fd;
}
close(fd);
rc = renameat2(dirfd, "empty", dirfd, "0", RENAME_WHITEOUT);
if (rc == -1) {
perror("renameat2");
return rc;
}
close(dirfd);
return 0;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: add selinux labels to whiteout inodes
2022-06-20 22:55 [PATCH] xfs: add selinux labels to whiteout inodes Eric Sandeen
2022-06-20 23:06 ` Eric Sandeen
@ 2022-06-20 23:41 ` Dave Chinner
2022-06-21 1:38 ` [PATCH V2] " Eric Sandeen
2 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2022-06-20 23:41 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Mon, Jun 20, 2022 at 05:55:31PM -0500, Eric Sandeen wrote:
> We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't
> apply an SELinux label on xfs" as it does on other filesystems
> (for example, ext4 and tmpfs.) While I'm not quite sure how labels
> may interact w/ whiteout files, leaving them as unlabeled seems
> inconsistent at best.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> fs/xfs/xfs_inode.c | 14 +++++++++++++-
> fs/xfs/xfs_iops.c | 2 +-
> fs/xfs/xfs_iops.h | 3 +++
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 52d6f2c..9a43060 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3046,10 +3046,12 @@ struct xfs_iunlink {
> static int
> xfs_rename_alloc_whiteout(
> struct user_namespace *mnt_userns,
> + struct xfs_name *src_name,
> struct xfs_inode *dp,
> struct xfs_inode **wip)
> {
> struct xfs_inode *tmpfile;
> + struct qstr name;
> int error;
>
> error = xfs_create_tmpfile(mnt_userns, dp, S_IFCHR | WHITEOUT_MODE,
> @@ -3057,6 +3059,15 @@ struct xfs_iunlink {
> if (error)
> return error;
>
> + name.name = src_name->name;
> + name.len = src_name->len;
> + error = xfs_init_security(VFS_I(tmpfile), VFS_I(dp), &name);
> + if (error) {
> + xfs_finish_inode_setup(tmpfile);
> + xfs_irele(tmpfile);
> + return error;
> + }
> +
I was worried that this would be inside an existing transaction,
but the tmpfile create is outside the rename transaction so this
will be fine.
> /*
> * Prepare the tmpfile inode as if it were created through the VFS.
> * Complete the inode setup and flag it as linkable. nlink is already
> @@ -3107,7 +3118,8 @@ struct xfs_iunlink {
> * appropriately.
> */
> if (flags & RENAME_WHITEOUT) {
> - error = xfs_rename_alloc_whiteout(mnt_userns, target_dp, &wip);
> + error = xfs_rename_alloc_whiteout(mnt_userns, src_name,
> + target_dp, &wip);
> if (error)
> return error;
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 29f5b8b8..c7775b7 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -76,7 +76,7 @@
> * inode, of course, such that log replay can't cause these to be lost).
> */
>
> -STATIC int
> +int
> xfs_init_security(
This function needs renaming, though. As a static function it can
get away with not having a namespace, but as a globally visible
function it needs to have an "xfs_inode_" prefix....
Otherwise OK.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] xfs: add selinux labels to whiteout inodes
2022-06-20 22:55 [PATCH] xfs: add selinux labels to whiteout inodes Eric Sandeen
2022-06-20 23:06 ` Eric Sandeen
2022-06-20 23:41 ` Dave Chinner
@ 2022-06-21 1:38 ` Eric Sandeen
2022-06-22 23:30 ` Darrick J. Wong
2 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2022-06-21 1:38 UTC (permalink / raw)
To: linux-xfs, david
We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't
apply an SELinux label on xfs" as it does on other filesystems
(for example, ext4 and tmpfs.) While I'm not quite sure how labels
may interact w/ whiteout files, leaving them as unlabeled seems
inconsistent at best. Now that xfs_init_security is not static,
rename it to xfs_inode_init_security per dchinner's suggestion.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_inode.c | 14 +++++++++++++-
fs/xfs/xfs_iops.c | 11 +++++------
fs/xfs/xfs_iops.h | 3 +++
3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 52d6f2c..58513a1 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3046,10 +3046,12 @@ struct xfs_iunlink {
static int
xfs_rename_alloc_whiteout(
struct user_namespace *mnt_userns,
+ struct xfs_name *src_name,
struct xfs_inode *dp,
struct xfs_inode **wip)
{
struct xfs_inode *tmpfile;
+ struct qstr name;
int error;
error = xfs_create_tmpfile(mnt_userns, dp, S_IFCHR | WHITEOUT_MODE,
@@ -3057,6 +3059,15 @@ struct xfs_iunlink {
if (error)
return error;
+ name.name = src_name->name;
+ name.len = src_name->len;
+ error = xfs_inode_init_security(VFS_I(tmpfile), VFS_I(dp), &name);
+ if (error) {
+ xfs_finish_inode_setup(tmpfile);
+ xfs_irele(tmpfile);
+ return error;
+ }
+
/*
* Prepare the tmpfile inode as if it were created through the VFS.
* Complete the inode setup and flag it as linkable. nlink is already
@@ -3107,7 +3118,8 @@ struct xfs_iunlink {
* appropriately.
*/
if (flags & RENAME_WHITEOUT) {
- error = xfs_rename_alloc_whiteout(mnt_userns, target_dp, &wip);
+ error = xfs_rename_alloc_whiteout(mnt_userns, src_name,
+ target_dp, &wip);
if (error)
return error;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 29f5b8b8..6720b60 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -75,9 +75,8 @@
* these attrs can be journalled at inode creation time (along with the
* inode, of course, such that log replay can't cause these to be lost).
*/
-
-STATIC int
-xfs_init_security(
+int
+xfs_inode_init_security(
struct inode *inode,
struct inode *dir,
const struct qstr *qstr)
@@ -122,7 +121,7 @@
/* Oh, the horror.
* If we can't add the ACL or we fail in
- * xfs_init_security we must back out.
+ * xfs_inode_init_security we must back out.
* ENOSPC can hit here, among other things.
*/
xfs_dentry_to_name(&teardown, dentry);
@@ -208,7 +207,7 @@
inode = VFS_I(ip);
- error = xfs_init_security(inode, dir, &dentry->d_name);
+ error = xfs_inode_init_security(inode, dir, &dentry->d_name);
if (unlikely(error))
goto out_cleanup_inode;
@@ -424,7 +423,7 @@
inode = VFS_I(cip);
- error = xfs_init_security(inode, dir, &dentry->d_name);
+ error = xfs_inode_init_security(inode, dir, &dentry->d_name);
if (unlikely(error))
goto out_cleanup_inode;
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index 2789490..cb5fc68 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -17,4 +17,7 @@
int xfs_vn_setattr_size(struct user_namespace *mnt_userns,
struct dentry *dentry, struct iattr *vap);
+int xfs_inode_init_security(struct inode *inode, struct inode *dir,
+ const struct qstr *qstr);
+
#endif /* __XFS_IOPS_H__ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] xfs: add selinux labels to whiteout inodes
2022-06-21 1:38 ` [PATCH V2] " Eric Sandeen
@ 2022-06-22 23:30 ` Darrick J. Wong
2022-06-22 23:41 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2022-06-22 23:30 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, david
On Mon, Jun 20, 2022 at 08:38:36PM -0500, Eric Sandeen wrote:
> We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't
> apply an SELinux label on xfs" as it does on other filesystems
> (for example, ext4 and tmpfs.) While I'm not quite sure how labels
> may interact w/ whiteout files, leaving them as unlabeled seems
> inconsistent at best. Now that xfs_init_security is not static,
> rename it to xfs_inode_init_security per dchinner's suggestion.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
Looks fine to me. I wondered slightly if the label creation needs to be
atomic with the file creation, but quickly realized that /never/
happens. Assuming this isn't high priority 5.19 stuff, I'll just roll
this into 5.20 if that's ok?
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> ---
> fs/xfs/xfs_inode.c | 14 +++++++++++++-
> fs/xfs/xfs_iops.c | 11 +++++------
> fs/xfs/xfs_iops.h | 3 +++
> 3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 52d6f2c..58513a1 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3046,10 +3046,12 @@ struct xfs_iunlink {
> static int
> xfs_rename_alloc_whiteout(
> struct user_namespace *mnt_userns,
> + struct xfs_name *src_name,
> struct xfs_inode *dp,
> struct xfs_inode **wip)
> {
> struct xfs_inode *tmpfile;
> + struct qstr name;
> int error;
>
> error = xfs_create_tmpfile(mnt_userns, dp, S_IFCHR | WHITEOUT_MODE,
> @@ -3057,6 +3059,15 @@ struct xfs_iunlink {
> if (error)
> return error;
>
> + name.name = src_name->name;
> + name.len = src_name->len;
> + error = xfs_inode_init_security(VFS_I(tmpfile), VFS_I(dp), &name);
> + if (error) {
> + xfs_finish_inode_setup(tmpfile);
> + xfs_irele(tmpfile);
> + return error;
> + }
> +
> /*
> * Prepare the tmpfile inode as if it were created through the VFS.
> * Complete the inode setup and flag it as linkable. nlink is already
> @@ -3107,7 +3118,8 @@ struct xfs_iunlink {
> * appropriately.
> */
> if (flags & RENAME_WHITEOUT) {
> - error = xfs_rename_alloc_whiteout(mnt_userns, target_dp, &wip);
> + error = xfs_rename_alloc_whiteout(mnt_userns, src_name,
> + target_dp, &wip);
> if (error)
> return error;
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 29f5b8b8..6720b60 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -75,9 +75,8 @@
> * these attrs can be journalled at inode creation time (along with the
> * inode, of course, such that log replay can't cause these to be lost).
> */
> -
> -STATIC int
> -xfs_init_security(
> +int
> +xfs_inode_init_security(
> struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr)
> @@ -122,7 +121,7 @@
>
> /* Oh, the horror.
> * If we can't add the ACL or we fail in
> - * xfs_init_security we must back out.
> + * xfs_inode_init_security we must back out.
> * ENOSPC can hit here, among other things.
> */
> xfs_dentry_to_name(&teardown, dentry);
> @@ -208,7 +207,7 @@
>
> inode = VFS_I(ip);
>
> - error = xfs_init_security(inode, dir, &dentry->d_name);
> + error = xfs_inode_init_security(inode, dir, &dentry->d_name);
> if (unlikely(error))
> goto out_cleanup_inode;
>
> @@ -424,7 +423,7 @@
>
> inode = VFS_I(cip);
>
> - error = xfs_init_security(inode, dir, &dentry->d_name);
> + error = xfs_inode_init_security(inode, dir, &dentry->d_name);
> if (unlikely(error))
> goto out_cleanup_inode;
>
> diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
> index 2789490..cb5fc68 100644
> --- a/fs/xfs/xfs_iops.h
> +++ b/fs/xfs/xfs_iops.h
> @@ -17,4 +17,7 @@
> int xfs_vn_setattr_size(struct user_namespace *mnt_userns,
> struct dentry *dentry, struct iattr *vap);
>
> +int xfs_inode_init_security(struct inode *inode, struct inode *dir,
> + const struct qstr *qstr);
> +
> #endif /* __XFS_IOPS_H__ */
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] xfs: add selinux labels to whiteout inodes
2022-06-22 23:30 ` Darrick J. Wong
@ 2022-06-22 23:41 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2022-06-22 23:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david
On 6/22/22 6:30 PM, Darrick J. Wong wrote:
> On Mon, Jun 20, 2022 at 08:38:36PM -0500, Eric Sandeen wrote:
>> We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't
>> apply an SELinux label on xfs" as it does on other filesystems
>> (for example, ext4 and tmpfs.) While I'm not quite sure how labels
>> may interact w/ whiteout files, leaving them as unlabeled seems
>> inconsistent at best. Now that xfs_init_security is not static,
>> rename it to xfs_inode_init_security per dchinner's suggestion.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>
> Looks fine to me. I wondered slightly if the label creation needs to be
> atomic with the file creation, but quickly realized that /never/
> happens. Assuming this isn't high priority 5.19 stuff, I'll just roll
> this into 5.20 if that's ok?
>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Thanks Darrick. I don't think it's high priority, I got a bug report about
the behavior, but there was no indication that it was actively causing
visible problems.
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-22 23:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-20 22:55 [PATCH] xfs: add selinux labels to whiteout inodes Eric Sandeen
2022-06-20 23:06 ` Eric Sandeen
2022-06-20 23:41 ` Dave Chinner
2022-06-21 1:38 ` [PATCH V2] " Eric Sandeen
2022-06-22 23:30 ` Darrick J. Wong
2022-06-22 23:41 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox