* Re: [PATCH 1/2] fs: hoist BTRFS_IOC_[SG]ET_FSLABEL to vfs
From: Andreas Dilger @ 2018-05-09 17:15 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Eric Sandeen, Eric Sandeen, fsdevel, linux-btrfs@vger.kernel.org,
Linux API
In-Reply-To: <20180509161043.GF4116@magnolia>
[-- Attachment #1: Type: text/plain, Size: 4954 bytes --]
On May 9, 2018, at 10:10 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, May 09, 2018 at 11:01:21AM -0500, Eric Sandeen wrote:
>> Move the btrfs label ioctls up to the vfs for general use.
>>
>> This retains 256 chars as the maximum size through the interface, which
>> is the btrfs limit and AFAIK exceeds any other filesystem's maximum
>> label size.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Let the bikeshedding on the exact ioctl name begin ;)
>>
>> fs/btrfs/ioctl.c | 8 ++++----
>> include/uapi/linux/btrfs.h | 6 ++----
>> include/uapi/linux/fs.h | 8 ++++++--
>> 3 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 632e26d..2dd4cdf 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -5444,6 +5444,10 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_setflags(file, argp);
>> case FS_IOC_GETVERSION:
>> return btrfs_ioctl_getversion(file, argp);
>> + case FS_IOC_GETFSLABEL:
>> + return btrfs_ioctl_get_fslabel(file, argp);
>> + case FS_IOC_SETFSLABEL:
>> + return btrfs_ioctl_set_fslabel(file, argp);
>> case FITRIM:
>> return btrfs_ioctl_fitrim(file, argp);
>> case BTRFS_IOC_SNAP_CREATE:
>> @@ -5555,10 +5559,6 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_quota_rescan_wait(file, argp);
>> case BTRFS_IOC_DEV_REPLACE:
>> return btrfs_ioctl_dev_replace(fs_info, argp);
>> - case BTRFS_IOC_GET_FSLABEL:
>> - return btrfs_ioctl_get_fslabel(file, argp);
>> - case BTRFS_IOC_SET_FSLABEL:
>> - return btrfs_ioctl_set_fslabel(file, argp);
>> case BTRFS_IOC_GET_SUPPORTED_FEATURES:
>> return btrfs_ioctl_get_supported_features(argp);
>> case BTRFS_IOC_GET_FEATURES:
>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
>> index c8d99b9..ec611c8 100644
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -823,10 +823,8 @@ enum btrfs_err_code {
>> #define BTRFS_IOC_QUOTA_RESCAN_STATUS _IOR(BTRFS_IOCTL_MAGIC, 45, \
>> struct btrfs_ioctl_quota_rescan_args)
>> #define BTRFS_IOC_QUOTA_RESCAN_WAIT _IO(BTRFS_IOCTL_MAGIC, 46)
>> -#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>> - char[BTRFS_LABEL_SIZE])
>> -#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
>> - char[BTRFS_LABEL_SIZE])
>> +#define BTRFS_IOC_GET_FSLABEL FS_IOC_GETFSLABEL
>> +#define BTRFS_IOC_SET_FSLABEL FS_IOC_SETFSLABEL
>> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>> struct btrfs_ioctl_get_dev_stats)
>> #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
>> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
>> index d2a8313..1df3707 100644
>> --- a/include/uapi/linux/fs.h
>> +++ b/include/uapi/linux/fs.h
>> @@ -242,6 +242,8 @@ struct fsxattr {
>> #define FICLONERANGE _IOW(0x94, 13, struct file_clone_range)
>> #define FIDEDUPERANGE _IOWR(0x94, 54, struct file_dedupe_range)
>>
>> +#define FSLABEL_MAX 256 /* Max chars for the interface; each fs may differ */
>> +
>> #define FS_IOC_GETFLAGS _IOR('f', 1, long)
>> #define FS_IOC_SETFLAGS _IOW('f', 2, long)
>> #define FS_IOC_GETVERSION _IOR('v', 1, long)
>> @@ -251,8 +253,10 @@ struct fsxattr {
>> #define FS_IOC32_SETFLAGS _IOW('f', 2, int)
>> #define FS_IOC32_GETVERSION _IOR('v', 1, int)
>> #define FS_IOC32_SETVERSION _IOW('v', 2, int)
>> -#define FS_IOC_FSGETXATTR _IOR ('X', 31, struct fsxattr)
>> -#define FS_IOC_FSSETXATTR _IOW ('X', 32, struct fsxattr)
>> +#define FS_IOC_FSGETXATTR _IOR('X', 31, struct fsxattr)
>> +#define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr)
>
> Separate patch for whitespace cleanup.
Really? I've heard Ted complain the other way, that whitespace cleanup
by itself is useless and should only be done as part of other changes.
As long as it is not overwhelming the rest of the patch I don't see an
issue with a minor improvement being part of another patch.
Otherwise, no bikeshedding from me. Looks very reasonable, and the 256-char
limit is definitely large enough IMHO (also matches the normal filename size
limit, so if the label is used as a pathname component there are no added
restrictions).
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
Cheers, Andreas
>> +#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX])
>> +#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX])
>
> Looks ok otherwise,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
>
>>
>> /*
>> * File system encryption support
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-api" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] man2: New page documenting filesystem get/set label ioctls
From: Darrick J. Wong @ 2018-05-09 16:15 UTC (permalink / raw)
To: Eric Sandeen
Cc: Eric Sandeen, fsdevel, linux-btrfs@vger.kernel.org, Linux API,
Michael Kerrisk (man-pages)
In-Reply-To: <708b8e2a-2bc2-df38-ec9c-c605203052b5@sandeen.net>
On Wed, May 09, 2018 at 11:04:03AM -0500, Eric Sandeen wrote:
> This documents the proposed new vfs-level ioctls which can
> get or set a mounted filesytem's label.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> btrfs folks, please verify that this accurately describes your
> current behavior, thanks.
>
> diff --git a/man2/ioctl_fslabel.2 b/man2/ioctl_fslabel.2
> new file mode 100644
> index 0000000..150aa53
> --- /dev/null
> +++ b/man2/ioctl_fslabel.2
> @@ -0,0 +1,83 @@
> +.\" Copyright (c) 2018, Red Hat, Inc. All rights reserved.
> +.\"
> +.\" %%%LICENSE_START(GPLv2+_DOC_FULL)
> +.\" This is free documentation; you can redistribute it and/or
> +.\" modify it under the terms of the GNU General Public License as
> +.\" published by the Free Software Foundation; either version 2 of
> +.\" the License, or (at your option) any later version.
> +.\"
> +.\" The GNU General Public License's references to "object code"
> +.\" and "executables" are to be interpreted as the output of any
> +.\" document formatting or typesetting system, including
> +.\" intermediate and printed output.
> +.\"
> +.\" This manual is distributed in the hope that it will be useful,
> +.\" but WITHOUT ANY WARRANTY; without even the implied warranty of
> +.\" MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +.\" GNU General Public License for more details.
> +.\"
> +.\" You should have received a copy of the GNU General Public
> +.\" License along with this manual; if not, see
> +.\" <http://www.gnu.org/licenses/>.
> +.\" %%%LICENSE_END
> +.TH IOCTL-FSLABEL 2 2018-05-02 "Linux" "Linux Programmer's Manual"
> +.SH NAME
> +ioctl_fslabel \- get or set a filesystem label
> +.SH SYNOPSIS
> +.br
> +.B #include <sys/ioctl.h>
> +.br
> +.B #include <linux/fs.h>
> +.sp
> +.BI "int ioctl(int " fd ", FS_IOC_GETFSLABEL, char " label [FSLABEL_MAX]);
> +.br
> +.BI "int ioctl(int " fd ", FS_IOC_SETFSLABEL, char " label [FSLABEL_MAX]);
> +.SH DESCRIPTION
> +If a filesystem supports online label manipulation, these
> +.BR ioctl (2)
> +operations can be used to get or set the filesystem label for the filesystem
> +on which
> +.B fd
> +resides.
Does the calling process need special capabilities or permissions? If
so, those should be listed here.
> +.SH RETURN VALUE
> +On success zero is returned. On error, \-1 is returned, and
> +.I errno
> +is set to indicate the error.
> +.PP
> +.SH ERRORS
> +Error codes can be one of, but are not limited to, the following:
> +.TP
> +.B EINVAL
> +The specified label exceeds the maximum label length for the filesystem.
> +.TP
> +.B ENOTTY
> +This can appear if the filesystem does not support online label manipulation.
> +.TP
> +.B EPERM
> +The calling process does not have sufficient permissions to set the label.
> +.TP
> +.B EFAULT
> +.I label
> +references an inaccessible memory area.
> +.SH VERSIONS
> +These ioctl operations first appeared in Linux 4.18.
> +They were previously known as
> +.B BTRFS_IOC_GET_FSLABEL
> +and
> +.B BTRFS_IOC_SET_FSLABEL
> +and were private to Btrfs.
> +.SH CONFORMING TO
> +This API is Linux-specific.
> +.SH NOTES
> +The maximum string length for this interface is
> +.BR FSLABEL_MAX ,
> +including the terminating null byte (\(aq\\0\(aq).
> +Filesystems have differing maximum label lengths, which may or
> +may not include the terminating null. The string provided to
> +.B FS_IOC_SETFSLABEL
> +must always be null-terminated, and the string returned by
> +.B FS_IOC_GETFSLABEL
> +will always be null-terminated.
> +.SH SEE ALSO
> +.BR ioctl (2),
> +.BR blkid (8)
> diff --git a/man2/ioctl_getfslabel.2 b/man2/ioctl_getfslabel.2
> new file mode 100644
> index 0000000..bfa8dca
> --- /dev/null
> +++ b/man2/ioctl_getfslabel.2
> @@ -0,0 +1 @@
> +.so man2/ioctl_fslabel.2
> diff --git a/man2/ioctl_setfslabel.2 b/man2/ioctl_setfslabel.2
> new file mode 100644
> index 0000000..bfa8dca
> --- /dev/null
> +++ b/man2/ioctl_setfslabel.2
> @@ -0,0 +1 @@
> +.so man2/ioctl_fslabel.2
Put all the manpage content into ioctl_getfslabel.2 and have
ioctl_setfslabel.2 point to it, rather than three files.
--D
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] fs: hoist BTRFS_IOC_[SG]ET_FSLABEL to vfs
From: Darrick J. Wong @ 2018-05-09 16:10 UTC (permalink / raw)
To: Eric Sandeen
Cc: Eric Sandeen, fsdevel, linux-btrfs@vger.kernel.org, Linux API
In-Reply-To: <b8c7feca-68ce-b1a8-3a96-a99b50457a84@sandeen.net>
On Wed, May 09, 2018 at 11:01:21AM -0500, Eric Sandeen wrote:
> Move the btrfs label ioctls up to the vfs for general use.
>
> This retains 256 chars as the maximum size through the interface, which
> is the btrfs limit and AFAIK exceeds any other filesystem's maximum
> label size.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Let the bikeshedding on the exact ioctl name begin ;)
>
> fs/btrfs/ioctl.c | 8 ++++----
> include/uapi/linux/btrfs.h | 6 ++----
> include/uapi/linux/fs.h | 8 ++++++--
> 3 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 632e26d..2dd4cdf 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5444,6 +5444,10 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_setflags(file, argp);
> case FS_IOC_GETVERSION:
> return btrfs_ioctl_getversion(file, argp);
> + case FS_IOC_GETFSLABEL:
> + return btrfs_ioctl_get_fslabel(file, argp);
> + case FS_IOC_SETFSLABEL:
> + return btrfs_ioctl_set_fslabel(file, argp);
> case FITRIM:
> return btrfs_ioctl_fitrim(file, argp);
> case BTRFS_IOC_SNAP_CREATE:
> @@ -5555,10 +5559,6 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_quota_rescan_wait(file, argp);
> case BTRFS_IOC_DEV_REPLACE:
> return btrfs_ioctl_dev_replace(fs_info, argp);
> - case BTRFS_IOC_GET_FSLABEL:
> - return btrfs_ioctl_get_fslabel(file, argp);
> - case BTRFS_IOC_SET_FSLABEL:
> - return btrfs_ioctl_set_fslabel(file, argp);
> case BTRFS_IOC_GET_SUPPORTED_FEATURES:
> return btrfs_ioctl_get_supported_features(argp);
> case BTRFS_IOC_GET_FEATURES:
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index c8d99b9..ec611c8 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -823,10 +823,8 @@ enum btrfs_err_code {
> #define BTRFS_IOC_QUOTA_RESCAN_STATUS _IOR(BTRFS_IOCTL_MAGIC, 45, \
> struct btrfs_ioctl_quota_rescan_args)
> #define BTRFS_IOC_QUOTA_RESCAN_WAIT _IO(BTRFS_IOCTL_MAGIC, 46)
> -#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
> - char[BTRFS_LABEL_SIZE])
> -#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
> - char[BTRFS_LABEL_SIZE])
> +#define BTRFS_IOC_GET_FSLABEL FS_IOC_GETFSLABEL
> +#define BTRFS_IOC_SET_FSLABEL FS_IOC_SETFSLABEL
> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
> struct btrfs_ioctl_get_dev_stats)
> #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index d2a8313..1df3707 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -242,6 +242,8 @@ struct fsxattr {
> #define FICLONERANGE _IOW(0x94, 13, struct file_clone_range)
> #define FIDEDUPERANGE _IOWR(0x94, 54, struct file_dedupe_range)
>
> +#define FSLABEL_MAX 256 /* Max chars for the interface; each fs may differ */
> +
> #define FS_IOC_GETFLAGS _IOR('f', 1, long)
> #define FS_IOC_SETFLAGS _IOW('f', 2, long)
> #define FS_IOC_GETVERSION _IOR('v', 1, long)
> @@ -251,8 +253,10 @@ struct fsxattr {
> #define FS_IOC32_SETFLAGS _IOW('f', 2, int)
> #define FS_IOC32_GETVERSION _IOR('v', 1, int)
> #define FS_IOC32_SETVERSION _IOW('v', 2, int)
> -#define FS_IOC_FSGETXATTR _IOR ('X', 31, struct fsxattr)
> -#define FS_IOC_FSSETXATTR _IOW ('X', 32, struct fsxattr)
> +#define FS_IOC_FSGETXATTR _IOR('X', 31, struct fsxattr)
> +#define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr)
Separate patch for whitespace cleanup.
> +#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX])
> +#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX])
Looks ok otherwise,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
>
> /*
> * File system encryption support
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 2/2] man2: New page documenting filesystem get/set label ioctls
From: Eric Sandeen @ 2018-05-09 16:04 UTC (permalink / raw)
To: Eric Sandeen, fsdevel, linux-btrfs@vger.kernel.org, Linux API,
Michael Kerrisk (man-pages)
In-Reply-To: <d2979d75-5e45-b145-9ca5-2c315d8ead9c@redhat.com>
This documents the proposed new vfs-level ioctls which can
get or set a mounted filesytem's label.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
btrfs folks, please verify that this accurately describes your
current behavior, thanks.
diff --git a/man2/ioctl_fslabel.2 b/man2/ioctl_fslabel.2
new file mode 100644
index 0000000..150aa53
--- /dev/null
+++ b/man2/ioctl_fslabel.2
@@ -0,0 +1,83 @@
+.\" Copyright (c) 2018, Red Hat, Inc. All rights reserved.
+.\"
+.\" %%%LICENSE_START(GPLv2+_DOC_FULL)
+.\" This is free documentation; you can redistribute it and/or
+.\" modify it under the terms of the GNU General Public License as
+.\" published by the Free Software Foundation; either version 2 of
+.\" the License, or (at your option) any later version.
+.\"
+.\" The GNU General Public License's references to "object code"
+.\" and "executables" are to be interpreted as the output of any
+.\" document formatting or typesetting system, including
+.\" intermediate and printed output.
+.\"
+.\" This manual is distributed in the hope that it will be useful,
+.\" but WITHOUT ANY WARRANTY; without even the implied warranty of
+.\" MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+.\" GNU General Public License for more details.
+.\"
+.\" You should have received a copy of the GNU General Public
+.\" License along with this manual; if not, see
+.\" <http://www.gnu.org/licenses/>.
+.\" %%%LICENSE_END
+.TH IOCTL-FSLABEL 2 2018-05-02 "Linux" "Linux Programmer's Manual"
+.SH NAME
+ioctl_fslabel \- get or set a filesystem label
+.SH SYNOPSIS
+.br
+.B #include <sys/ioctl.h>
+.br
+.B #include <linux/fs.h>
+.sp
+.BI "int ioctl(int " fd ", FS_IOC_GETFSLABEL, char " label [FSLABEL_MAX]);
+.br
+.BI "int ioctl(int " fd ", FS_IOC_SETFSLABEL, char " label [FSLABEL_MAX]);
+.SH DESCRIPTION
+If a filesystem supports online label manipulation, these
+.BR ioctl (2)
+operations can be used to get or set the filesystem label for the filesystem
+on which
+.B fd
+resides.
+.SH RETURN VALUE
+On success zero is returned. On error, \-1 is returned, and
+.I errno
+is set to indicate the error.
+.PP
+.SH ERRORS
+Error codes can be one of, but are not limited to, the following:
+.TP
+.B EINVAL
+The specified label exceeds the maximum label length for the filesystem.
+.TP
+.B ENOTTY
+This can appear if the filesystem does not support online label manipulation.
+.TP
+.B EPERM
+The calling process does not have sufficient permissions to set the label.
+.TP
+.B EFAULT
+.I label
+references an inaccessible memory area.
+.SH VERSIONS
+These ioctl operations first appeared in Linux 4.18.
+They were previously known as
+.B BTRFS_IOC_GET_FSLABEL
+and
+.B BTRFS_IOC_SET_FSLABEL
+and were private to Btrfs.
+.SH CONFORMING TO
+This API is Linux-specific.
+.SH NOTES
+The maximum string length for this interface is
+.BR FSLABEL_MAX ,
+including the terminating null byte (\(aq\\0\(aq).
+Filesystems have differing maximum label lengths, which may or
+may not include the terminating null. The string provided to
+.B FS_IOC_SETFSLABEL
+must always be null-terminated, and the string returned by
+.B FS_IOC_GETFSLABEL
+will always be null-terminated.
+.SH SEE ALSO
+.BR ioctl (2),
+.BR blkid (8)
diff --git a/man2/ioctl_getfslabel.2 b/man2/ioctl_getfslabel.2
new file mode 100644
index 0000000..bfa8dca
--- /dev/null
+++ b/man2/ioctl_getfslabel.2
@@ -0,0 +1 @@
+.so man2/ioctl_fslabel.2
diff --git a/man2/ioctl_setfslabel.2 b/man2/ioctl_setfslabel.2
new file mode 100644
index 0000000..bfa8dca
--- /dev/null
+++ b/man2/ioctl_setfslabel.2
@@ -0,0 +1 @@
+.so man2/ioctl_fslabel.2
^ permalink raw reply related
* [PATCH 1/2] fs: hoist BTRFS_IOC_[SG]ET_FSLABEL to vfs
From: Eric Sandeen @ 2018-05-09 16:01 UTC (permalink / raw)
To: Eric Sandeen, fsdevel, linux-btrfs@vger.kernel.org, Linux API
In-Reply-To: <d2979d75-5e45-b145-9ca5-2c315d8ead9c@redhat.com>
Move the btrfs label ioctls up to the vfs for general use.
This retains 256 chars as the maximum size through the interface, which
is the btrfs limit and AFAIK exceeds any other filesystem's maximum
label size.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
Let the bikeshedding on the exact ioctl name begin ;)
fs/btrfs/ioctl.c | 8 ++++----
include/uapi/linux/btrfs.h | 6 ++----
include/uapi/linux/fs.h | 8 ++++++--
3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 632e26d..2dd4cdf 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5444,6 +5444,10 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_setflags(file, argp);
case FS_IOC_GETVERSION:
return btrfs_ioctl_getversion(file, argp);
+ case FS_IOC_GETFSLABEL:
+ return btrfs_ioctl_get_fslabel(file, argp);
+ case FS_IOC_SETFSLABEL:
+ return btrfs_ioctl_set_fslabel(file, argp);
case FITRIM:
return btrfs_ioctl_fitrim(file, argp);
case BTRFS_IOC_SNAP_CREATE:
@@ -5555,10 +5559,6 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_quota_rescan_wait(file, argp);
case BTRFS_IOC_DEV_REPLACE:
return btrfs_ioctl_dev_replace(fs_info, argp);
- case BTRFS_IOC_GET_FSLABEL:
- return btrfs_ioctl_get_fslabel(file, argp);
- case BTRFS_IOC_SET_FSLABEL:
- return btrfs_ioctl_set_fslabel(file, argp);
case BTRFS_IOC_GET_SUPPORTED_FEATURES:
return btrfs_ioctl_get_supported_features(argp);
case BTRFS_IOC_GET_FEATURES:
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index c8d99b9..ec611c8 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -823,10 +823,8 @@ enum btrfs_err_code {
#define BTRFS_IOC_QUOTA_RESCAN_STATUS _IOR(BTRFS_IOCTL_MAGIC, 45, \
struct btrfs_ioctl_quota_rescan_args)
#define BTRFS_IOC_QUOTA_RESCAN_WAIT _IO(BTRFS_IOCTL_MAGIC, 46)
-#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
- char[BTRFS_LABEL_SIZE])
-#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
- char[BTRFS_LABEL_SIZE])
+#define BTRFS_IOC_GET_FSLABEL FS_IOC_GETFSLABEL
+#define BTRFS_IOC_SET_FSLABEL FS_IOC_SETFSLABEL
#define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
struct btrfs_ioctl_get_dev_stats)
#define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index d2a8313..1df3707 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -242,6 +242,8 @@ struct fsxattr {
#define FICLONERANGE _IOW(0x94, 13, struct file_clone_range)
#define FIDEDUPERANGE _IOWR(0x94, 54, struct file_dedupe_range)
+#define FSLABEL_MAX 256 /* Max chars for the interface; each fs may differ */
+
#define FS_IOC_GETFLAGS _IOR('f', 1, long)
#define FS_IOC_SETFLAGS _IOW('f', 2, long)
#define FS_IOC_GETVERSION _IOR('v', 1, long)
@@ -251,8 +253,10 @@ struct fsxattr {
#define FS_IOC32_SETFLAGS _IOW('f', 2, int)
#define FS_IOC32_GETVERSION _IOR('v', 1, int)
#define FS_IOC32_SETVERSION _IOW('v', 2, int)
-#define FS_IOC_FSGETXATTR _IOR ('X', 31, struct fsxattr)
-#define FS_IOC_FSSETXATTR _IOW ('X', 32, struct fsxattr)
+#define FS_IOC_FSGETXATTR _IOR('X', 31, struct fsxattr)
+#define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr)
+#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX])
+#define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX])
/*
* File system encryption support
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] hoist btrfs' label set/get ioctls to vfs, and document
From: Eric Sandeen @ 2018-05-09 15:56 UTC (permalink / raw)
To: fsdevel, linux-btrfs@vger.kernel.org, Linux API
I'm planning to add online label set/get support to xfs, and to do so
I plan to re-use the existing btrfs ioctls, BTRFS_IOC_[SG]ET_FSLABEL
We're still working out minor details on the xfs side, but I'd like to
start the conversation regarding the new more generic interface ASAP,
so here goes - patches to move the ioctls to the vfs and document them.
(Other filesystems may wish to use this interface in the future as well)
Thanks,
-Eric
^ permalink raw reply
* Re: [PATCH v3 2/3] fs: Convert kiocb rw_hint from enum to u16
From: Adam Manzanares @ 2018-05-09 15:29 UTC (permalink / raw)
To: Jens Axboe, viro@zeniv.linux.org.uk, bcrl@kvack.org,
linux-fsdevel@vger.kernel.org, linux-aio@kvack.org,
linux-api@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180509152149.GG28388@thunk.org>
On 5/9/18 11:21 AM, Theodore Y. Ts'o wrote:
> On Wed, May 09, 2018 at 08:23:00AM -0600, Jens Axboe wrote:
>> Streams is essentially the only thing ki_hint is currently used for,
>> with the write life time hints mapping to a stream. The idea for the
>> user side API was to have other things than just write life time hints.
>>
>> Since Adam wants to do priorities, he'd either need to pack into the
>> existing ki_hint, or do this patch does, which is make it smaller and
>> add a new member. I think the latter is cleaner.
>
> Fair enough; but maybe we can use a u8 instead of a u16? 65,535
> priorities still seem like way more than would ever make sense. I
> think 256 priorities is still way to many, but it's simpler while
> still reserving number of bits for future se.
The intention was to mimic the ioprio_set system call, which uses 3 bits
for a prio class and 13 bits for a prio_value.
IDK what is the right amount of bits to use, but the existing use of
bits seemed flexible enough to support many types of applications and
devices.
>
> - Ted
>
^ permalink raw reply
* Re: [PATCH v3 2/3] fs: Convert kiocb rw_hint from enum to u16
From: Theodore Y. Ts'o @ 2018-05-09 15:21 UTC (permalink / raw)
To: Jens Axboe
Cc: adam.manzanares, viro, bcrl, linux-fsdevel, linux-aio, linux-api,
linux-block, linux-kernel
In-Reply-To: <0aad993b-448c-3586-cfc0-f2e50b1dda28@kernel.dk>
On Wed, May 09, 2018 at 08:23:00AM -0600, Jens Axboe wrote:
> Streams is essentially the only thing ki_hint is currently used for,
> with the write life time hints mapping to a stream. The idea for the
> user side API was to have other things than just write life time hints.
>
> Since Adam wants to do priorities, he'd either need to pack into the
> existing ki_hint, or do this patch does, which is make it smaller and
> add a new member. I think the latter is cleaner.
Fair enough; but maybe we can use a u8 instead of a u16? 65,535
priorities still seem like way more than would ever make sense. I
think 256 priorities is still way to many, but it's simpler while
still reserving number of bits for future se.
- Ted
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Andy Lutomirski @ 2018-05-09 14:41 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-arch, Linux-MM, Linux API, X86 ML, linuxram, Dave Hansen,
linux-x86_64, Andrew Lutomirski, linuxppc-dev
In-Reply-To: <314e1a48-db94-9b37-8793-a95a2082c9e2@redhat.com>
On Tue, May 8, 2018 at 5:40 AM Florian Weimer <fweimer@redhat.com> wrote:
> On 05/08/2018 04:49 AM, Andy Lutomirski wrote:
> > On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com>
wrote:
> >
> >> On 05/03/2018 06:05 AM, Andy Lutomirski wrote:
> >>> On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote:
> >>>
> >>>> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote:
> >>>>>
> >>>>>> If I recall correctly, the POWER maintainer did express a strong
> >>> desire
> >>>>>> back then for (what is, I believe) their current semantics, which
my
> >>>>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too.
> >>>>>
> >>>>> Ram, I really really don't like the POWER semantics. Can you give
> > some
> >>>>> justification for them? Does POWER at least have an atomic way for
> >>>>> userspace to modify just the key it wants to modify or, even better,
> >>>>> special load and store instructions to use alternate keys?
> >>>
> >>>> I wouldn't call it POWER semantics. The way I implemented it on power
> >>>> lead to the semantics, given that nothing was explicitly stated
> >>>> about how the semantics should work within a signal handler.
> >>>
> >>> I think that this is further evidence that we should introduce a new
> >>> pkey_alloc() mode and deprecate the old. To the extent possible, this
> >>> thing should work the same way on x86 and POWER.
> >
> >> Do you propose to change POWER or to change x86?
> >
> > Sorry for being slow to reply. I propose to introduce a new
> > PKEY_ALLOC_something variant on x86 and POWER and to make the behavior
> > match on both.
> So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the
> existing (different) behavior without the flag?
> Ram, would you be okay with that? Could you give me a hand if
> necessary? (I assume we have silicon in-house because it's a
> long-standing feature of the POWER platform which was simply dormant on
> Linux until now.)
> > It should at least update the values loaded when a signal
> > is delivered and it should probably also update it for new threads.
> I think we should keep inheritance for new threads and fork. pkey_alloc
> only has a single access rights argument, which makes it hard to reuse
> this interface if there are two (three) separate sets of access rights.
Hmm. I can get on board with the idea that fork() / clone() /
pthread_create() are all just special cases of the idea that the thread
that *calls* them should have the right pkey values, and the latter is
already busted given our inability to asynchronously propagate the new mode
in pkey_alloc(). So let's so PKEY_ALLOC_SETSIGNAL as a starting point.
One thing we could do, though: the current initual state on process
creation is all access blocked on all keys. We could change it so that
half the keys are fully blocked and half are read-only. Then we could add
a PKEY_ALLOC_STRICT or similar that allocates a key with the correct
initial state *and* does the setsignal thing. If there are no keys left
with the correct initial state, then it fails.
^ permalink raw reply
* Re: [PATCH v3 2/3] fs: Convert kiocb rw_hint from enum to u16
From: Jens Axboe @ 2018-05-09 14:23 UTC (permalink / raw)
To: Theodore Y. Ts'o, adam.manzanares, viro, bcrl, linux-fsdevel,
linux-aio, linux-api, linux-block, linux-kernel
In-Reply-To: <20180509133432.GD28388@thunk.org>
On 5/9/18 7:34 AM, Theodore Y. Ts'o wrote:
> On Tue, May 08, 2018 at 10:42:01AM -0700, adam.manzanares@wdc.com wrote:
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 760d8da1b6c7..7a90ce387e00 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -284,6 +284,8 @@ enum rw_hint {
>> WRITE_LIFE_EXTREME = RWH_WRITE_LIFE_EXTREME,
>> };
>>
>> +#define MAX_KI_HINT ((1 << 16) - 1) /* ki_hint type is u16 */
>> +
>
> Do we really think there will be *ever* be a need for more than 16 I/O
> priority levels? I would much rather use the low four bits of KI_HINT
> for the priority level, and reserve the rest of the 16 bits in KI_HINT
> for some future use. (For example, we might want to use some number
> of bits for a stream ID.)
Streams is essentially the only thing ki_hint is currently used for,
with the write life time hints mapping to a stream. The idea for the
user side API was to have other things than just write life time hints.
Since Adam wants to do priorities, he'd either need to pack into the
existing ki_hint, or do this patch does, which is make it smaller and
add a new member. I think the latter is cleaner.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v3 2/3] fs: Convert kiocb rw_hint from enum to u16
From: Theodore Y. Ts'o @ 2018-05-09 13:34 UTC (permalink / raw)
To: adam.manzanares
Cc: viro, bcrl, linux-fsdevel, linux-aio, linux-api, linux-block,
linux-kernel
In-Reply-To: <20180508174202.2537-3-adam.manzanares@wdc.com>
On Tue, May 08, 2018 at 10:42:01AM -0700, adam.manzanares@wdc.com wrote:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..7a90ce387e00 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -284,6 +284,8 @@ enum rw_hint {
> WRITE_LIFE_EXTREME = RWH_WRITE_LIFE_EXTREME,
> };
>
> +#define MAX_KI_HINT ((1 << 16) - 1) /* ki_hint type is u16 */
> +
Do we really think there will be *ever* be a need for more than 16 I/O
priority levels? I would much rather use the low four bits of KI_HINT
for the priority level, and reserve the rest of the 16 bits in KI_HINT
for some future use. (For example, we might want to use some number
of bits for a stream ID.)
- Ted
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v3 3/3] fs: Add aio iopriority support for block_dev
From: kbuild test robot @ 2018-05-09 8:54 UTC (permalink / raw)
Cc: kbuild-all, viro, bcrl, linux-fsdevel, linux-aio, linux-api,
linux-block, linux-kernel, Adam Manzanares
In-Reply-To: <20180508174202.2537-4-adam.manzanares@wdc.com>
[-- Attachment #1: Type: text/plain, Size: 4828 bytes --]
Hi Adam,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc4 next-20180508]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/adam-manzanares-wdc-com/block-add-ioprio_check_cap-function/20180509-094058
config: x86_64-randconfig-s0-05091255 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
fs/aio.c: In function 'io_submit_one':
>> fs/aio.c:1606:9: error: implicit declaration of function 'ioprio_check_cap' [-Werror=implicit-function-declaration]
ret = ioprio_check_cap(iocb->aio_reqprio);
^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/ioprio_check_cap +1606 fs/aio.c
1545
1546 static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
1547 struct iocb *iocb, bool compat)
1548 {
1549 struct aio_kiocb *req;
1550 struct file *file;
1551 ssize_t ret;
1552
1553 /* enforce forwards compatibility on users */
1554 if (unlikely(iocb->aio_reserved2)) {
1555 pr_debug("EINVAL: reserve field set\n");
1556 return -EINVAL;
1557 }
1558
1559 /* prevent overflows */
1560 if (unlikely(
1561 (iocb->aio_buf != (unsigned long)iocb->aio_buf) ||
1562 (iocb->aio_nbytes != (size_t)iocb->aio_nbytes) ||
1563 ((ssize_t)iocb->aio_nbytes < 0)
1564 )) {
1565 pr_debug("EINVAL: overflow check\n");
1566 return -EINVAL;
1567 }
1568
1569 req = aio_get_req(ctx);
1570 if (unlikely(!req))
1571 return -EAGAIN;
1572
1573 req->common.ki_filp = file = fget(iocb->aio_fildes);
1574 if (unlikely(!req->common.ki_filp)) {
1575 ret = -EBADF;
1576 goto out_put_req;
1577 }
1578 req->common.ki_pos = iocb->aio_offset;
1579 req->common.ki_complete = aio_complete;
1580 req->common.ki_flags = iocb_flags(req->common.ki_filp);
1581 req->common.ki_hint = file_write_hint(file);
1582
1583 if (iocb->aio_flags & IOCB_FLAG_RESFD) {
1584 /*
1585 * If the IOCB_FLAG_RESFD flag of aio_flags is set, get an
1586 * instance of the file* now. The file descriptor must be
1587 * an eventfd() fd, and will be signaled for each completed
1588 * event using the eventfd_signal() function.
1589 */
1590 req->ki_eventfd = eventfd_ctx_fdget((int) iocb->aio_resfd);
1591 if (IS_ERR(req->ki_eventfd)) {
1592 ret = PTR_ERR(req->ki_eventfd);
1593 req->ki_eventfd = NULL;
1594 goto out_put_req;
1595 }
1596
1597 req->common.ki_flags |= IOCB_EVENTFD;
1598 }
1599
1600 if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
1601 /*
1602 * If the IOCB_FLAG_IOPRIO flag of aio_flags is set, then
1603 * aio_reqprio is interpreted as an I/O scheduling
1604 * class and priority.
1605 */
> 1606 ret = ioprio_check_cap(iocb->aio_reqprio);
1607 if (ret) {
1608 pr_debug("aio ioprio check cap error\n");
1609 goto out_put_req;
1610 }
1611
1612 req->common.ki_ioprio = iocb->aio_reqprio;
1613 req->common.ki_flags |= IOCB_IOPRIO;
1614 }
1615
1616 ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
1617 if (unlikely(ret)) {
1618 pr_debug("EINVAL: aio_rw_flags\n");
1619 goto out_put_req;
1620 }
1621
1622 ret = put_user(KIOCB_KEY, &user_iocb->aio_key);
1623 if (unlikely(ret)) {
1624 pr_debug("EFAULT: aio_key\n");
1625 goto out_put_req;
1626 }
1627
1628 req->ki_user_iocb = user_iocb;
1629 req->ki_user_data = iocb->aio_data;
1630
1631 get_file(file);
1632 switch (iocb->aio_lio_opcode) {
1633 case IOCB_CMD_PREAD:
1634 ret = aio_read(&req->common, iocb, false, compat);
1635 break;
1636 case IOCB_CMD_PWRITE:
1637 ret = aio_write(&req->common, iocb, false, compat);
1638 break;
1639 case IOCB_CMD_PREADV:
1640 ret = aio_read(&req->common, iocb, true, compat);
1641 break;
1642 case IOCB_CMD_PWRITEV:
1643 ret = aio_write(&req->common, iocb, true, compat);
1644 break;
1645 default:
1646 pr_debug("invalid aio operation %d\n", iocb->aio_lio_opcode);
1647 ret = -EINVAL;
1648 break;
1649 }
1650 fput(file);
1651
1652 if (ret && ret != -EIOCBQUEUED)
1653 goto out_put_req;
1654 return 0;
1655 out_put_req:
1656 put_reqs_available(ctx, 1);
1657 percpu_ref_put(&ctx->reqs);
1658 kiocb_free(req);
1659 return ret;
1660 }
1661
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30767 bytes --]
^ permalink raw reply
* Re: [RFC] vfs: skip extra attributes check on removal for symlinks
From: Darrick J. Wong @ 2018-05-09 1:39 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: viro, linux-fsdevel, linux-api, sandeen, dhowells, tytso, fliu,
jack, jeffm, nborisov, jake.norris, mtk.manpages, linux-xfs,
linux-kernel
In-Reply-To: <20180509003855.GK27853@wotan.suse.de>
On Wed, May 09, 2018 at 12:38:55AM +0000, Luis R. Rodriguez wrote:
> On Tue, May 08, 2018 at 05:17:41PM -0700, Darrick J. Wong wrote:
> > On Wed, May 09, 2018 at 12:03:28AM +0000, Luis R. Rodriguez wrote:
> > > Groovy, thanks, let's not forget the xfs_repair respective fix :) let me know
> > > if you have any feedback on that.
> >
> > TBH I've lost any proposed xfs_repair patches to the mists of time
> > because some patch volcano keeps erupting on the lists. :P
> >
> > Uh... I think it's fine for xfs_{repair,scrub} to clear the immutable
> > and append flags on any special inodes it finds, particularly since
> > neither flag has any real meaning for block/char/fifo/socket/symlinks
> > anyway.
>
> Sure, my point during review of that series for xfs_repair in particular
> though was that for symlinks the justification is different (half of the
> commit log in this new patch), as such I'd prefer to deal with them in a
> separate follow up patch.
Ah, found it again finally[1]. ISTR the discussion petered out after
Dave asked if you had a map of inode mode
(file/dir/bdev/cdev/fifo/socket/symlink) to allowable flags. That would
be a good general way to detect and clear stray flags.
--D
[1] https://marc.info/?l=linux-xfs&m=150948985429374&w=4
> Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] vfs: skip extra attributes check on removal for symlinks
From: Luis R. Rodriguez @ 2018-05-09 0:38 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Luis R. Rodriguez, viro, linux-fsdevel, linux-api, sandeen,
dhowells, tytso, fliu, jack, jeffm, nborisov, jake.norris,
mtk.manpages, linux-xfs, linux-kernel
In-Reply-To: <20180509001741.GB11261@magnolia>
On Tue, May 08, 2018 at 05:17:41PM -0700, Darrick J. Wong wrote:
> On Wed, May 09, 2018 at 12:03:28AM +0000, Luis R. Rodriguez wrote:
> > Groovy, thanks, let's not forget the xfs_repair respective fix :) let me know
> > if you have any feedback on that.
>
> TBH I've lost any proposed xfs_repair patches to the mists of time
> because some patch volcano keeps erupting on the lists. :P
>
> Uh... I think it's fine for xfs_{repair,scrub} to clear the immutable
> and append flags on any special inodes it finds, particularly since
> neither flag has any real meaning for block/char/fifo/socket/symlinks
> anyway.
Sure, my point during review of that series for xfs_repair in particular
though was that for symlinks the justification is different (half of the
commit log in this new patch), as such I'd prefer to deal with them in a
separate follow up patch.
Luis
^ permalink raw reply
* Re: [RFC] vfs: skip extra attributes check on removal for symlinks
From: Darrick J. Wong @ 2018-05-09 0:17 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: viro, linux-fsdevel, linux-api, sandeen, dhowells, tytso, fliu,
jack, jeffm, nborisov, jake.norris, mtk.manpages, linux-xfs,
linux-kernel
In-Reply-To: <20180509000327.GJ27853@wotan.suse.de>
On Wed, May 09, 2018 at 12:03:28AM +0000, Luis R. Rodriguez wrote:
> On Mon, May 07, 2018 at 05:30:55PM -0700, Darrick J. Wong wrote:
> > On Tue, May 01, 2018 at 05:45:12PM +0000, Luis R. Rodriguez wrote:
> > > On Tue, May 01, 2018 at 10:23:19AM -0700, Darrick J. Wong wrote:
> > > > On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote:
> > > > > Linux filesystems cannot set extra file attributes (stx_attributes as per
> > > > > statx(2)) on a symbolic link. To set extra file attributes you issue
> > > > > ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link
> > > > > yield EBADF.
> > > > >
> > > > > This is because ioctl(2) tries to obtain struct fd from the symbolic link
> > > > > file descriptor passed using fdget(), fdget() in turn always returns no
> > > > > file set when a file descriptor is open with O_PATH. As per symlink(2)
> > > > > O_PATH and O_NOFOLLOW must *always* be used when you want to get the file
> > > > > descriptor of a symbolic link, and this holds true for Linux, as such extra
> > > > > file attributes cannot possibly be set on symbolic links on Linux.
> > > > >
> > > > > Filesystems repair utilities should be updated to detect this as
> > > > > corruption and correct this, however, the VFS *does* respect these
> > > > > extra attributes on symlinks for removal.
> > > > >
> > > > > Since we cannot set these attributes we should special-case the
> > > > > immutable/append on delete for symlinks, this would be consistent with
> > > > > what we *do* allow on Linux for all filesystems.
> > > >
> > > > Ah, ok, so the problem here is that you can't rm an "immutable" symlink
> > > > nor can you clear the immutable flag on such a beast, so therefore
> > > > ignore the immutable (and append) flags if we're trying to delete a
> > > > symlink?
> > >
> > > Yup.
> > >
> > > > I think we ought to teach the xfs inode verifier to check for
> > > > immutable/append symlinks and return error so that we don't end up with
> > > > such things in core in the first place,
> > >
> > > Agreed. But note that one way to end up with these things is through
> > > corruption. Once a user finds this the first signs they'll run into
> > > (unless they have the awesome new online scrubber) is they cannot delete
> > > some odd file and not know why. And then they'll see they cannot change
> > > or remove either the immutable or append flag. The immutable attribute
> > > is known to not let you delete the file, but its less known that the
> > > append attribute implies the same. Folks would scratch their heads.
> > >
> > > Since one cannot *set* these attributes on symlinks on Linux, other than
> > > ignoring such attributes perhaps we should warn about it? As the only
> > > way you could end up with that is if your filesystem got corrupted.
> > >
> > > But without filesystems having a fix for that merged users can't do anything.
> > >
> > > > and fix xfs_repair to zap such things.
> > >
> > > This is what my patch for xfs_repair does, which is pending review.
> > >
> > > But note that special files are handled differently, I explain the logic on the
> > > RFC commit log, which is why I suggested splitting up adding a fix for special
> > > files as a separate patch.
> > >
> > > > That said, for the filesystems that aren't going to check their inodes,
> > > > I guess this is a (hackish) way to avoid presenting undeletable gunk in
> > > > the fs to the user...
> > >
> > > That's why its RFC. What is the right thing to do here?
> >
> > Ideally, none of the filesystems should ever feed garbage to the vfs,
> > but it's not all that obvious exactly what things the vfs will trip over.
> > And knowing that a lot of the filesystems do minimal checking if any, I
> > guess I'll just say that...
> >
> > ...XFS (and probably ext4) should catch immutable symlinks in the inode
> > verifiers so that xfs_iget returns -EFSCORRUPTED. For the rest of the
> > filesystems, it's probably fine to let them delete the symlink since the
> > user wants to kill it anyway.
>
> Alrighty, I have this implemented now.
>
> > > My own logic here was since we cannot possibly allow extended attributes on
> > > symlinks is to not use them then as well, in this case for delete.
> > >
> > > > (Were it up to me I'd make a common vfs_check_inode() to reject
> > > > struct inode containing garbage that the vfs won't deal with,
> > >
> > > There certainly are cases that we could come up with for the VFS where
> > > if such things are found, regardless of the filesystem, we're very sure
> > > its a corruption. This is just one example, as you note.
> > >
> > > So it is correct that there are two things here:
> > >
> > > 1) Do we respect the attribute for symlink on delete
> > > 2) Do we warn to users of the fact that the inode is very likely corrupted
> > > regardless of the filesystem?
> >
> > But I suppose we could WARN_ON_ONCE to state that we're allowing
> > deletion of an immutable symlink that we couldn't possibly have set.
>
> Yes, I think that's better than to allow filesystems to do things even
> though the VFS in this case *knows* better.
>
> > Anyway, couple this patch with a second one to fix the xfs verifier and
> > I'll be happy.
>
> Groovy, thanks, let's not forget the xfs_repair respective fix :) let me know
> if you have any feedback on that.
TBH I've lost any proposed xfs_repair patches to the mists of time
because some patch volcano keeps erupting on the lists. :P
Uh... I think it's fine for xfs_{repair,scrub} to clear the immutable
and append flags on any special inodes it finds, particularly since
neither flag has any real meaning for block/char/fifo/socket/symlinks
anyway.
(Well ok I could imagine immutable pipes being meaningful for
ignoring^Wdealing with the bureaucracy but I don't see a non-joke
meaning. :P)
--D
> Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] vfs: skip extra attributes check on removal for symlinks
From: Luis R. Rodriguez @ 2018-05-09 0:03 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Luis R. Rodriguez, viro, linux-fsdevel, linux-api, sandeen,
dhowells, tytso, fliu, jack, jeffm, nborisov, jake.norris,
mtk.manpages, linux-xfs, linux-kernel
In-Reply-To: <20180508003055.GC11261@magnolia>
On Mon, May 07, 2018 at 05:30:55PM -0700, Darrick J. Wong wrote:
> On Tue, May 01, 2018 at 05:45:12PM +0000, Luis R. Rodriguez wrote:
> > On Tue, May 01, 2018 at 10:23:19AM -0700, Darrick J. Wong wrote:
> > > On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote:
> > > > Linux filesystems cannot set extra file attributes (stx_attributes as per
> > > > statx(2)) on a symbolic link. To set extra file attributes you issue
> > > > ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link
> > > > yield EBADF.
> > > >
> > > > This is because ioctl(2) tries to obtain struct fd from the symbolic link
> > > > file descriptor passed using fdget(), fdget() in turn always returns no
> > > > file set when a file descriptor is open with O_PATH. As per symlink(2)
> > > > O_PATH and O_NOFOLLOW must *always* be used when you want to get the file
> > > > descriptor of a symbolic link, and this holds true for Linux, as such extra
> > > > file attributes cannot possibly be set on symbolic links on Linux.
> > > >
> > > > Filesystems repair utilities should be updated to detect this as
> > > > corruption and correct this, however, the VFS *does* respect these
> > > > extra attributes on symlinks for removal.
> > > >
> > > > Since we cannot set these attributes we should special-case the
> > > > immutable/append on delete for symlinks, this would be consistent with
> > > > what we *do* allow on Linux for all filesystems.
> > >
> > > Ah, ok, so the problem here is that you can't rm an "immutable" symlink
> > > nor can you clear the immutable flag on such a beast, so therefore
> > > ignore the immutable (and append) flags if we're trying to delete a
> > > symlink?
> >
> > Yup.
> >
> > > I think we ought to teach the xfs inode verifier to check for
> > > immutable/append symlinks and return error so that we don't end up with
> > > such things in core in the first place,
> >
> > Agreed. But note that one way to end up with these things is through
> > corruption. Once a user finds this the first signs they'll run into
> > (unless they have the awesome new online scrubber) is they cannot delete
> > some odd file and not know why. And then they'll see they cannot change
> > or remove either the immutable or append flag. The immutable attribute
> > is known to not let you delete the file, but its less known that the
> > append attribute implies the same. Folks would scratch their heads.
> >
> > Since one cannot *set* these attributes on symlinks on Linux, other than
> > ignoring such attributes perhaps we should warn about it? As the only
> > way you could end up with that is if your filesystem got corrupted.
> >
> > But without filesystems having a fix for that merged users can't do anything.
> >
> > > and fix xfs_repair to zap such things.
> >
> > This is what my patch for xfs_repair does, which is pending review.
> >
> > But note that special files are handled differently, I explain the logic on the
> > RFC commit log, which is why I suggested splitting up adding a fix for special
> > files as a separate patch.
> >
> > > That said, for the filesystems that aren't going to check their inodes,
> > > I guess this is a (hackish) way to avoid presenting undeletable gunk in
> > > the fs to the user...
> >
> > That's why its RFC. What is the right thing to do here?
>
> Ideally, none of the filesystems should ever feed garbage to the vfs,
> but it's not all that obvious exactly what things the vfs will trip over.
> And knowing that a lot of the filesystems do minimal checking if any, I
> guess I'll just say that...
>
> ...XFS (and probably ext4) should catch immutable symlinks in the inode
> verifiers so that xfs_iget returns -EFSCORRUPTED. For the rest of the
> filesystems, it's probably fine to let them delete the symlink since the
> user wants to kill it anyway.
Alrighty, I have this implemented now.
> > My own logic here was since we cannot possibly allow extended attributes on
> > symlinks is to not use them then as well, in this case for delete.
> >
> > > (Were it up to me I'd make a common vfs_check_inode() to reject
> > > struct inode containing garbage that the vfs won't deal with,
> >
> > There certainly are cases that we could come up with for the VFS where
> > if such things are found, regardless of the filesystem, we're very sure
> > its a corruption. This is just one example, as you note.
> >
> > So it is correct that there are two things here:
> >
> > 1) Do we respect the attribute for symlink on delete
> > 2) Do we warn to users of the fact that the inode is very likely corrupted
> > regardless of the filesystem?
>
> But I suppose we could WARN_ON_ONCE to state that we're allowing
> deletion of an immutable symlink that we couldn't possibly have set.
Yes, I think that's better than to allow filesystems to do things even
though the VFS in this case *knows* better.
> Anyway, couple this patch with a second one to fix the xfs verifier and
> I'll be happy.
Groovy, thanks, let's not forget the xfs_repair respective fix :) let me know
if you have any feedback on that.
Luis
^ permalink raw reply
* [PATCH v3 3/3] fs: Add aio iopriority support for block_dev
From: adam.manzanares @ 2018-05-08 17:42 UTC (permalink / raw)
To: viro, bcrl
Cc: linux-fsdevel, linux-aio, linux-api, linux-block, linux-kernel,
Adam Manzanares
In-Reply-To: <20180508174202.2537-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
When IOCB_FLAG_IOPRIO is set on the iocb aio_flags field, then we set the
newly added kiocb ki_ioprio field to the value in the iocb aio_reqprio field.
When a bio is created for an aio request by the block dev we set the priority
value of the bio to the user supplied value.
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
fs/aio.c | 16 ++++++++++++++++
fs/block_dev.c | 2 ++
include/linux/fs.h | 2 ++
include/uapi/linux/aio_abi.h | 1 +
4 files changed, 21 insertions(+)
diff --git a/fs/aio.c b/fs/aio.c
index 88d7927ffbc6..f43d1d3a2e39 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1597,6 +1597,22 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
req->common.ki_flags |= IOCB_EVENTFD;
}
+ if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
+ /*
+ * If the IOCB_FLAG_IOPRIO flag of aio_flags is set, then
+ * aio_reqprio is interpreted as an I/O scheduling
+ * class and priority.
+ */
+ ret = ioprio_check_cap(iocb->aio_reqprio);
+ if (ret) {
+ pr_debug("aio ioprio check cap error\n");
+ goto out_put_req;
+ }
+
+ req->common.ki_ioprio = iocb->aio_reqprio;
+ req->common.ki_flags |= IOCB_IOPRIO;
+ }
+
ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
if (unlikely(ret)) {
pr_debug("EINVAL: aio_rw_flags\n");
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..970bef79caa6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -355,6 +355,8 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
bio->bi_write_hint = iocb->ki_hint;
bio->bi_private = dio;
bio->bi_end_io = blkdev_bio_end_io;
+ if (iocb->ki_flags & IOCB_IOPRIO)
+ bio->bi_ioprio = iocb->ki_ioprio;
ret = bio_iov_iter_get_pages(bio, iter);
if (unlikely(ret)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7a90ce387e00..3415e83f6350 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -294,6 +294,7 @@ enum rw_hint {
#define IOCB_SYNC (1 << 5)
#define IOCB_WRITE (1 << 6)
#define IOCB_NOWAIT (1 << 7)
+#define IOCB_IOPRIO (1 << 8)
struct kiocb {
struct file *ki_filp;
@@ -302,6 +303,7 @@ struct kiocb {
void *private;
int ki_flags;
u16 ki_hint;
+ u16 ki_ioprio; /* See linux/ioprio.h */
} __randomize_layout;
static inline bool is_sync_kiocb(struct kiocb *kiocb)
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index a04adbc70ddf..d4593a6062ef 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -54,6 +54,7 @@ enum {
* is valid.
*/
#define IOCB_FLAG_RESFD (1 << 0)
+#define IOCB_FLAG_IOPRIO (1 << 1)
/* read() from /dev/aio returns these structures. */
struct io_event {
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH v3 2/3] fs: Convert kiocb rw_hint from enum to u16
From: adam.manzanares @ 2018-05-08 17:42 UTC (permalink / raw)
To: viro, bcrl
Cc: linux-fsdevel, linux-aio, linux-api, linux-block, linux-kernel,
Adam Manzanares
In-Reply-To: <20180508174202.2537-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
In order to avoid kiocb bloat for per command iopriority support, rw_hint
is converted from enum to a u16. Added a guard around ki_hint assigment.
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
include/linux/fs.h | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..7a90ce387e00 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -284,6 +284,8 @@ enum rw_hint {
WRITE_LIFE_EXTREME = RWH_WRITE_LIFE_EXTREME,
};
+#define MAX_KI_HINT ((1 << 16) - 1) /* ki_hint type is u16 */
+
#define IOCB_EVENTFD (1 << 0)
#define IOCB_APPEND (1 << 1)
#define IOCB_DIRECT (1 << 2)
@@ -299,7 +301,7 @@ struct kiocb {
void (*ki_complete)(struct kiocb *iocb, long ret, long ret2);
void *private;
int ki_flags;
- enum rw_hint ki_hint;
+ u16 ki_hint;
} __randomize_layout;
static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -1927,12 +1929,21 @@ static inline enum rw_hint file_write_hint(struct file *file)
static inline int iocb_flags(struct file *file);
+/* ki_hint changed from enum to u16, make sure rw_hint fits into u16 */
+static inline u16 ki_hint_valid(enum rw_hint hint)
+{
+ if (hint > MAX_KI_HINT)
+ return 0;
+
+ return hint;
+}
+
static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
{
*kiocb = (struct kiocb) {
.ki_filp = filp,
.ki_flags = iocb_flags(filp),
- .ki_hint = file_write_hint(filp),
+ .ki_hint = ki_hint_valid(file_write_hint(filp)),
};
}
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH v3 1/3] block: add ioprio_check_cap function
From: adam.manzanares @ 2018-05-08 17:42 UTC (permalink / raw)
To: viro, bcrl
Cc: linux-fsdevel, linux-aio, linux-api, linux-block, linux-kernel,
Adam Manzanares
In-Reply-To: <20180508174202.2537-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
Aio per command iopriority support introduces a second interface between
userland and the kernel capable of passing iopriority. The aio interface also
needs the ability to verify that the submitting context has sufficient
priviledges to submit IOPRIO_RT commands. This patch creates the
ioprio_check_cap function to be used by the ioprio_set system call and also by
the aio interface.
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
block/ioprio.c | 22 ++++++++++++++++------
include/linux/ioprio.h | 2 ++
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/block/ioprio.c b/block/ioprio.c
index 6f5d0b6625e3..f9821080c92c 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -61,15 +61,10 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
}
EXPORT_SYMBOL_GPL(set_task_ioprio);
-SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+int ioprio_check_cap(int ioprio)
{
int class = IOPRIO_PRIO_CLASS(ioprio);
int data = IOPRIO_PRIO_DATA(ioprio);
- struct task_struct *p, *g;
- struct user_struct *user;
- struct pid *pgrp;
- kuid_t uid;
- int ret;
switch (class) {
case IOPRIO_CLASS_RT:
@@ -92,6 +87,21 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
return -EINVAL;
}
+ return 0;
+}
+
+SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+{
+ struct task_struct *p, *g;
+ struct user_struct *user;
+ struct pid *pgrp;
+ kuid_t uid;
+ int ret;
+
+ ret = ioprio_check_cap(ioprio);
+ if (ret)
+ return ret;
+
ret = -ESRCH;
rcu_read_lock();
switch (which) {
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 627efac73e6d..4a28cec49ec3 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -77,4 +77,6 @@ extern int ioprio_best(unsigned short aprio, unsigned short bprio);
extern int set_task_ioprio(struct task_struct *task, int ioprio);
+extern int ioprio_check_cap(int ioprio);
+
#endif
--
2.15.1
^ permalink raw reply related
* [PATCH v3 0/3] AIO add per-command iopriority
From: adam.manzanares @ 2018-05-08 17:41 UTC (permalink / raw)
To: viro, bcrl
Cc: linux-fsdevel, linux-aio, linux-api, linux-block, linux-kernel,
Adam Manzanares
From: Adam Manzanares <adam.manzanares@wdc.com>
This is the per-I/O equivalent of the ioprio_set system call.
See the following link for performance implications on a SATA HDD:
https://lkml.org/lkml/2016/12/6/495
First patch factors ioprio_check_cap function out of ioprio_set system call to
also be used by the aio ioprio interface.
Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.
Third patch passes ioprio hint from aio iocb to kiocb and enables block_dev
usage of the per I/O ioprio feature.
v2: merge patches
use IOCB_FLAG_IOPRIO
validate intended use with IOCB_IOPRIO
add linux-api and linux-block to cc
v3: add ioprio_check_cap function
convert kiocb ki_hint to u16
use ioprio_check_cap when adding ioprio to kiocb in aio.c
Adam Manzanares (3):
block: add ioprio_check_cap function
fs: Convert kiocb rw_hint from enum to u16
fs: Add aio iopriority support for block_dev
block/ioprio.c | 22 ++++++++++++++++------
fs/aio.c | 16 ++++++++++++++++
fs/block_dev.c | 2 ++
include/linux/fs.h | 17 +++++++++++++++--
include/linux/ioprio.h | 2 ++
include/uapi/linux/aio_abi.h | 1 +
6 files changed, 52 insertions(+), 8 deletions(-)
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Christopher Lameter @ 2018-05-08 12:53 UTC (permalink / raw)
To: prakash.sangappa
Cc: linux-kernel, linux-mm, linux-api, akpm, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes
In-Reply-To: <a61bc7b6-ed98-045d-95c0-b6c91fc8d1da@oracle.com>
On Mon, 7 May 2018, prakash.sangappa wrote:
> > each address for each page? Length of the VMA segment?
> > Physical address?
>
> Need numa node information for each virtual address with pages mapped.
> No need of physical address.
You need per page information? Note that there can only be one page
per virtual address. Or are we talking about address ranges?
https://www.kernel.org/doc/Documentation/vm/pagemap.txt ?
Also the move_pages syscall has the ability to determine the location of
individual pages.
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Florian Weimer @ 2018-05-08 12:40 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-arch, Linux-MM, Linux API, X86 ML, linuxram, Dave Hansen,
linux-x86_64, linuxppc-dev
In-Reply-To: <CALCETrX46wR_MDW=m9SVm=ejQmPAmD3+2oC3iapf75bPhnEAWQ@mail.gmail.com>
On 05/08/2018 04:49 AM, Andy Lutomirski wrote:
> On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote:
>
>> On 05/03/2018 06:05 AM, Andy Lutomirski wrote:
>>> On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote:
>>>
>>>> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote:
>>>>>
>>>>>> If I recall correctly, the POWER maintainer did express a strong
>>> desire
>>>>>> back then for (what is, I believe) their current semantics, which my
>>>>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too.
>>>>>
>>>>> Ram, I really really don't like the POWER semantics. Can you give
> some
>>>>> justification for them? Does POWER at least have an atomic way for
>>>>> userspace to modify just the key it wants to modify or, even better,
>>>>> special load and store instructions to use alternate keys?
>>>
>>>> I wouldn't call it POWER semantics. The way I implemented it on power
>>>> lead to the semantics, given that nothing was explicitly stated
>>>> about how the semantics should work within a signal handler.
>>>
>>> I think that this is further evidence that we should introduce a new
>>> pkey_alloc() mode and deprecate the old. To the extent possible, this
>>> thing should work the same way on x86 and POWER.
>
>> Do you propose to change POWER or to change x86?
>
> Sorry for being slow to reply. I propose to introduce a new
> PKEY_ALLOC_something variant on x86 and POWER and to make the behavior
> match on both.
So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the
existing (different) behavior without the flag?
Ram, would you be okay with that? Could you give me a hand if
necessary? (I assume we have silicon in-house because it's a
long-standing feature of the POWER platform which was simply dormant on
Linux until now.)
> It should at least update the values loaded when a signal
> is delivered and it should probably also update it for new threads.
I think we should keep inheritance for new threads and fork. pkey_alloc
only has a single access rights argument, which makes it hard to reuse
this interface if there are two (three) separate sets of access rights.
Is there precedent for process state reverting on fork, besides
MADV_WIPEONFORK? My gut feeling is that we should avoid that.
> For glibc, for example, I assume that you want signals to be delivered with
> write access disabled to the GOT. Otherwise you would fail to protect
> against exploits that occur in signal context. Glibc controls thread
> creation, so the initial state on thread startup doesn't really matter, but
> there will be more users than just glibc.
glibc does not control thread, or more precisely, subprocess creation.
Otherwise we wouldn't have face that many issues with our PID cache. 8-/
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Andy Lutomirski @ 2018-05-08 2:49 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-arch, Linux-MM, Linux API, X86 ML, linuxram, Dave Hansen,
linux-x86_64, Andrew Lutomirski, linuxppc-dev
In-Reply-To: <927c8325-4c98-d7af-b921-6aafcf8fe992@redhat.com>
On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote:
> On 05/03/2018 06:05 AM, Andy Lutomirski wrote:
> > On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote:
> >
> >> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote:
> >>>
> >>>> If I recall correctly, the POWER maintainer did express a strong
> > desire
> >>>> back then for (what is, I believe) their current semantics, which my
> >>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too.
> >>>
> >>> Ram, I really really don't like the POWER semantics. Can you give
some
> >>> justification for them? Does POWER at least have an atomic way for
> >>> userspace to modify just the key it wants to modify or, even better,
> >>> special load and store instructions to use alternate keys?
> >
> >> I wouldn't call it POWER semantics. The way I implemented it on power
> >> lead to the semantics, given that nothing was explicitly stated
> >> about how the semantics should work within a signal handler.
> >
> > I think that this is further evidence that we should introduce a new
> > pkey_alloc() mode and deprecate the old. To the extent possible, this
> > thing should work the same way on x86 and POWER.
> Do you propose to change POWER or to change x86?
Sorry for being slow to reply. I propose to introduce a new
PKEY_ALLOC_something variant on x86 and POWER and to make the behavior
match on both. It should at least update the values loaded when a signal
is delivered and it should probably also update it for new threads.
For glibc, for example, I assume that you want signals to be delivered with
write access disabled to the GOT. Otherwise you would fail to protect
against exploits that occur in signal context. Glibc controls thread
creation, so the initial state on thread startup doesn't really matter, but
there will be more users than just glibc.
--Andy
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-05-08 1:16 UTC (permalink / raw)
To: Dave Hansen, Anshuman Khandual, Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi
In-Reply-To: <c72fea44-59f3-b106-8311-b5eae2d254e7@intel.com>
On 05/07/2018 05:05 PM, Dave Hansen wrote:
> On 05/07/2018 04:22 PM, prakash.sangappa wrote:
>> However, with the proposed new file, we could allow seeking to
>> specified virtual address. The lseek offset in this case would
>> represent the virtual address of the process. Subsequent read from
>> the file would provide VA range to numa node information starting
>> from that VA. In case the VA seek'ed to is invalid, it will start
>> from the next valid mapped VA of the process. The implementation
>> would not be based on seq_file.
> So you're proposing a new /proc/<pid> file that appears next to and is
> named very similarly to the exiting /proc/<pid>, but which has entirely
> different behavior?
It will be /proc/<pid>/numa_vamaps. Yes, the behavior will be
different with respect to seeking. Output will still be text and
the format will be same.
I want to get feedback on this approach.
^ permalink raw reply
* Re: [RFC] vfs: skip extra attributes check on removal for symlinks
From: Darrick J. Wong @ 2018-05-08 0:30 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: viro, linux-fsdevel, linux-api, sandeen, dhowells, tytso, fliu,
jack, jeffm, nborisov, jake.norris, mtk.manpages, linux-xfs,
linux-kernel
In-Reply-To: <20180501174512.GA27853@wotan.suse.de>
On Tue, May 01, 2018 at 05:45:12PM +0000, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 10:23:19AM -0700, Darrick J. Wong wrote:
> > On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote:
> > > Linux filesystems cannot set extra file attributes (stx_attributes as per
> > > statx(2)) on a symbolic link. To set extra file attributes you issue
> > > ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link
> > > yield EBADF.
> > >
> > > This is because ioctl(2) tries to obtain struct fd from the symbolic link
> > > file descriptor passed using fdget(), fdget() in turn always returns no
> > > file set when a file descriptor is open with O_PATH. As per symlink(2)
> > > O_PATH and O_NOFOLLOW must *always* be used when you want to get the file
> > > descriptor of a symbolic link, and this holds true for Linux, as such extra
> > > file attributes cannot possibly be set on symbolic links on Linux.
> > >
> > > Filesystems repair utilities should be updated to detect this as
> > > corruption and correct this, however, the VFS *does* respect these
> > > extra attributes on symlinks for removal.
> > >
> > > Since we cannot set these attributes we should special-case the
> > > immutable/append on delete for symlinks, this would be consistent with
> > > what we *do* allow on Linux for all filesystems.
> >
> > Ah, ok, so the problem here is that you can't rm an "immutable" symlink
> > nor can you clear the immutable flag on such a beast, so therefore
> > ignore the immutable (and append) flags if we're trying to delete a
> > symlink?
>
> Yup.
>
> > I think we ought to teach the xfs inode verifier to check for
> > immutable/append symlinks and return error so that we don't end up with
> > such things in core in the first place,
>
> Agreed. But note that one way to end up with these things is through
> corruption. Once a user finds this the first signs they'll run into
> (unless they have the awesome new online scrubber) is they cannot delete
> some odd file and not know why. And then they'll see they cannot change
> or remove either the immutable or append flag. The immutable attribute
> is known to not let you delete the file, but its less known that the
> append attribute implies the same. Folks would scratch their heads.
>
> Since one cannot *set* these attributes on symlinks on Linux, other than
> ignoring such attributes perhaps we should warn about it? As the only
> way you could end up with that is if your filesystem got corrupted.
>
> But without filesystems having a fix for that merged users can't do anything.
>
> > and fix xfs_repair to zap such things.
>
> This is what my patch for xfs_repair does, which is pending review.
>
> But note that special files are handled differently, I explain the logic on the
> RFC commit log, which is why I suggested splitting up adding a fix for special
> files as a separate patch.
>
> > That said, for the filesystems that aren't going to check their inodes,
> > I guess this is a (hackish) way to avoid presenting undeletable gunk in
> > the fs to the user...
>
> That's why its RFC. What is the right thing to do here?
Ideally, none of the filesystems should ever feed garbage to the vfs,
but it's not all that obvious exactly what things the vfs will trip over.
And knowing that a lot of the filesystems do minimal checking if any, I
guess I'll just say that...
...XFS (and probably ext4) should catch immutable symlinks in the inode
verifiers so that xfs_iget returns -EFSCORRUPTED. For the rest of the
filesystems, it's probably fine to let them delete the symlink since the
user wants to kill it anyway.
> My own logic here was since we cannot possibly allow extended attributes on
> symlinks is to not use them then as well, in this case for delete.
>
> > (Were it up to me I'd make a common vfs_check_inode() to reject
> > struct inode containing garbage that the vfs won't deal with,
>
> There certainly are cases that we could come up with for the VFS where
> if such things are found, regardless of the filesystem, we're very sure
> its a corruption. This is just one example, as you note.
>
> So it is correct that there are two things here:
>
> 1) Do we respect the attribute for symlink on delete
> 2) Do we warn to users of the fact that the inode is very likely corrupted
> regardless of the filesystem?
But I suppose we could WARN_ON_ONCE to state that we're allowing
deletion of an immutable symlink that we couldn't possibly have set.
Anyway, couple this patch with a second one to fix the xfs verifier and
I'll be happy.
--D
> This patch only addresses 1) and makes it match the VFS expectation, and
> I think this is safe if we're not doing 2). This is why I'm also looking
> for feedback from linux-api folks.
>
> This is one of those odd corner cases we run into, and very likely not
> well documented anywhere.
>
> > and teach the filesystems to use it;
>
> Or the VFS uses it.
>
> Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox