| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z1rQp8UZz9s+BQM1@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Thu, 12 Dec 2024 17:31:43 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
Ritesh Harjani <ritesh.list@...il.com>, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org,
Andrey Albershteyn <aalbersh@...nel.org>,
John Garry <john.g.garry@...cle.com>
Subject: Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR
details
On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > Currently with stat we only show FS_IOC_FSGETXATTR details
> > if the filesystem is XFS. With extsize support also coming
> > to ext4 make sure to show these details when -c "stat" or "statx"
> > is used.
> >
> > No functional changes for filesystems other than ext4.
> >
> > Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> > ---
> > io/stat.c | 38 +++++++++++++++++++++-----------------
> > 1 file changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/io/stat.c b/io/stat.c
> > index 326f2822e276..d06c2186cde4 100644
> > --- a/io/stat.c
> > +++ b/io/stat.c
> > @@ -97,14 +97,14 @@ print_file_info(void)
> > file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> > }
> >
> > -static void
> > -print_xfs_info(int verbose)
> > +static void print_extended_info(int verbose)
> > {
> > - struct dioattr dio;
> > - struct fsxattr fsx, fsxa;
> > + struct dioattr dio;
> > + struct fsxattr fsx, fsxa;
> > + bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> >
> > - if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > - (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > + if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > + (is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
>
> Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> print whatever is returned, no matter what filesystem we think is
> feeding us information?
>
> e.g.
>
> if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> if (is_xfs_fd || (errno != EOPNOTSUPP &&
> errno != ENOTTY))
> perror("FS_IOC_GETXATTR");
> } else {
> printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> ...
> }
>
> if (ioctl(file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> if (is_xfs_fd || (errno != EOPNOTSUPP &&
> errno != ENOTTY))
> perror("XFS_IOC_FSGETXATTRA");
> } else {
> printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> }
>
> That way we don't have to specialcase platform_test_*_fd() for every
> other filesystem that might want to return real fsxattr results?
> Same idea for DIOINFO.
Hi Darrick, thanks for the review.
I agree that this looks like a more modular approach, I'll make the
change. IIUC we basically want to perform the ioctls regardless of the
FS and then handle the error/output accordingly here so that we wont need the
if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
return 0;
line in the stat functions, right?
Also, with the suggested approach the user visible behavior might change
subtly because earlier we used to fail if either FSGETXATTR or
FGGETXATTRA failed but now theres a slim chance that we might print the
output partially. It might not be a very big deal but just thought I'd
point out.
Regards,
ojaswin
>
> --D
>
> > perror("FS_IOC_FSGETXATTR");
> > } else {
> > printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> > @@ -113,14 +113,18 @@ print_xfs_info(int verbose)
> > printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
> > printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
> > printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);
> > - printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> > + if (is_xfs_fd)
> > + printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> > }
> > - if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> > - perror("XFS_IOC_DIOINFO");
> > - } else {
> > - printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> > - printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> > - printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> > +
> > + if (is_xfs_fd) {
> > + if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> > + perror("XFS_IOC_DIOINFO");
> > + } else {
> > + printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> > + printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> > + printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> > + }
> > }
> > }
> >
> > @@ -167,10 +171,10 @@ stat_f(
> > printf(_("stat.ctime = %s"), ctime(&st.st_ctime));
> > }
> >
> > - if (file->flags & IO_FOREIGN)
> > + if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
> > return 0;
> >
> > - print_xfs_info(verbose);
> > + print_extended_info(verbose);
> >
> > return 0;
> > }
> > @@ -440,10 +444,10 @@ statx_f(
> > ctime((time_t *)&stx.stx_btime.tv_sec));
> > }
> >
> > - if (file->flags & IO_FOREIGN)
> > + if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
> > return 0;
> >
> > - print_xfs_info(verbose);
> > + print_extended_info(verbose);
> >
> > return 0;
> > }
> > --
> > 2.43.5
> >
> >
Powered by blists - more mailing lists