consequences of XFS_IOC_FSSETXATTR on non-empty file?

consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Samuel Just]

Hi,

We are seeing reports of ceph-osd stores on xfs of files with some
garbage data (possibly misplaced from data elsewhere in the
filesystem).  There was a bug for a while where the ceph-osd process
would set a value for fsx_extsize on a non-empty (possibly sparse)
file using XFS_IOC_FSSETXATTR.  Could that plausibly result in a file
with garbage data?  Let me know if there is any additional information
or detail which might be valuable.  One report was on kernel versions
3.14.3 and 3.14.4.
-Sam

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Dave Chinner]

On Sat, Jul 12, 2014 at 06:16:54PM -0700, Samuel Just wrote:
> Hi,
> 
> We are seeing reports of ceph-osd stores on xfs of files with some
> garbage data (possibly misplaced from data elsewhere in the
> filesystem).  There was a bug for a while where the ceph-osd process
> would set a value for fsx_extsize on a non-empty (possibly sparse)
> file using XFS_IOC_FSSETXATTR.  Could that plausibly result in a file
> with garbage data? 

No, setting an extent size on a non-empty file will simply fail
with EINVAL.

Do you have any method of reproducing the bad data in files?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Samuel Just]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

Actually, on this ubuntu kernel (3.13.0-24-generic), it doesn't seem
to give an error.  I'll attach my test case for that.  We don't yet
have a way of reproducing the corruption -- the ext_size change in the
osd simply seemed like a promising lead.
-Sam

On Sat, Jul 12, 2014 at 6:26 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Sat, Jul 12, 2014 at 06:16:54PM -0700, Samuel Just wrote:
>> Hi,
>>
>> We are seeing reports of ceph-osd stores on xfs of files with some
>> garbage data (possibly misplaced from data elsewhere in the
>> filesystem).  There was a bug for a while where the ceph-osd process
>> would set a value for fsx_extsize on a non-empty (possibly sparse)
>> file using XFS_IOC_FSSETXATTR.  Could that plausibly result in a file
>> with garbage data?
>
> No, setting an extent size on a non-empty file will simply fail
> with EINVAL.
>
> Do you have any method of reproducing the bad data in files?
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com

[-- Attachment #2: xfs_extsize_test.cc --]
[-- Type: text/x-c++src, Size: 1642 bytes --]

/**
 * Try changing extsize on a non-empty sparse object
 */

#include <xfs/xfs.h>
#include <cstdio>
#include <unistd.h>
#include <iostream>
#include <string.h>

int main() {
  char buf[256];
  memset(buf, 1, sizeof(buf));

  int fd = open("test", O_RDWR|O_CREAT|O_EXCL, 0666);
  assert(fd >= 0);

  int r = pwrite(fd, buf, 1, 24<<10);
  assert(r == 1);

  close(fd);

  fd = open("test", O_RDWR);
  assert(fd >= 0);

  struct fsxattr fsx;
  r = ioctl(fd, XFS_IOC_FSGETXATTR, &fsx);
  if (r < 0) {
    int ret = -errno;
    std::cerr << "FSGETXATTR: " << ret << std::endl;
    return ret;
  }

  // already set?
  if (fsx.fsx_xflags & XFS_XFLAG_EXTSIZE) {
    std::cerr << "already set" << std::endl;
    return 0;
  }

  std::cerr << fsx.fsx_nextents << " exents, extsize is " << fsx.fsx_extsize << std::endl;

  unsigned val = 4<<20;
  fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
  fsx.fsx_extsize = val;
  
  if (ioctl(fd, XFS_IOC_FSSETXATTR, &fsx) < 0) {
    int ret = -errno;
    std::cerr << "FSSETXATTR: " << ret << std::endl;
    return ret;
  }

  struct fsxattr fsx2;
  r = ioctl(fd, XFS_IOC_FSGETXATTR, &fsx2);
  if (r < 0) {
    int ret = -errno;
    std::cerr << "FSGETXATTR: " << ret << std::endl;
    return ret;
  }

  if (fsx2.fsx_xflags & XFS_XFLAG_EXTSIZE) {
    std::cerr << "successfully set to " << fsx2.fsx_extsize << std::endl;
  }

  close(fd);

#if 0
  fd = open("test", O_RDONLY);
  assert(fd >= 0);

  char zbuf[24<<10];
  memset(zbuf, 0, sizeof(zbuf));
  char obuf[24<<10];
  r = read(fd, obuf, sizeof(obuf));
  assert(r == sizeof(obuf));

  r = memcmp(zbuf, obuf, sizeof(zbuf));
  assert(r == 0);
  close(fd);
#endif
}

[-- Attachment #3: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Ilya Dryomov]

On Sun, Jul 13, 2014 at 5:48 AM, Samuel Just <sam.just@inktank.com> wrote:
> Actually, on this ubuntu kernel (3.13.0-24-generic), it doesn't seem
> to give an error.  I'll attach my test case for that.  We don't yet
> have a way of reproducing the corruption -- the ext_size change in the
> osd simply seemed like a promising lead.
> -Sam
>
> On Sat, Jul 12, 2014 at 6:26 PM, Dave Chinner <david@fromorbit.com> wrote:
>> On Sat, Jul 12, 2014 at 06:16:54PM -0700, Samuel Just wrote:
>>> Hi,
>>>
>>> We are seeing reports of ceph-osd stores on xfs of files with some
>>> garbage data (possibly misplaced from data elsewhere in the
>>> filesystem).  There was a bug for a while where the ceph-osd process
>>> would set a value for fsx_extsize on a non-empty (possibly sparse)
>>> file using XFS_IOC_FSSETXATTR.  Could that plausibly result in a file
>>> with garbage data?
>>
>> No, setting an extent size on a non-empty file will simply fail
>> with EINVAL.

AFAIR it checks whether or not any extents are actually allocated, not
whether the file is empty or not.  I think if you call fsync() or even
fdatasync() before close(fd), it will fail as expected.

Thanks,

                Ilya

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Dave Chinner]

On Sun, Jul 13, 2014 at 09:01:13PM +0400, Ilya Dryomov wrote:
> On Sun, Jul 13, 2014 at 5:48 AM, Samuel Just <sam.just@inktank.com> wrote:
> > Actually, on this ubuntu kernel (3.13.0-24-generic), it doesn't seem
> > to give an error.  I'll attach my test case for that.  We don't yet
> > have a way of reproducing the corruption -- the ext_size change in the
> > osd simply seemed like a promising lead.
> > -Sam
> >
> > On Sat, Jul 12, 2014 at 6:26 PM, Dave Chinner <david@fromorbit.com> wrote:
> >> On Sat, Jul 12, 2014 at 06:16:54PM -0700, Samuel Just wrote:
> >>> Hi,
> >>>
> >>> We are seeing reports of ceph-osd stores on xfs of files with some
> >>> garbage data (possibly misplaced from data elsewhere in the
> >>> filesystem).  There was a bug for a while where the ceph-osd process
> >>> would set a value for fsx_extsize on a non-empty (possibly sparse)
> >>> file using XFS_IOC_FSSETXATTR.  Could that plausibly result in a file
> >>> with garbage data?
> >>
> >> No, setting an extent size on a non-empty file will simply fail
> >> with EINVAL.
> 
> AFAIR it checks whether or not any extents are actually allocated, not
> whether the file is empty or not.

FWIW, that's an implementation detail, not the definition of the
intended behaviour of the ioctl.  Indeed, the man page says:

"The fsx_xflags realtime file bit and the file's extent size may be
changed only when the file is empty, ..."

For most people, "[non-]empty file" is much more easily understood
than "a file without real extents, but might have been written to
and so have dirty, in-memory delayed allocation data whose
asynchronous flushing may or may not affect the behaviour of a call
to XFS_IOC_FSSETXATTR".

i.e. the intended application behaviour is that they should only be
able to change the extent size hint *before* any data is written to
the file.

> I think if you call fsync() or even
> fdatasync() before close(fd), it will fail as expected.

Only if you are trying to change the extent size immediately after
the first write you do to an empty file. Which is, as per the above,
not the recommended or intended use of the ioctl.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Ilya Dryomov]

On Mon, Jul 14, 2014 at 2:55 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Sun, Jul 13, 2014 at 09:01:13PM +0400, Ilya Dryomov wrote:
>> On Sun, Jul 13, 2014 at 5:48 AM, Samuel Just <sam.just@inktank.com> wrote:
>> > Actually, on this ubuntu kernel (3.13.0-24-generic), it doesn't seem
>> > to give an error.  I'll attach my test case for that.  We don't yet
>> > have a way of reproducing the corruption -- the ext_size change in the
>> > osd simply seemed like a promising lead.
>> > -Sam
>> >
>> > On Sat, Jul 12, 2014 at 6:26 PM, Dave Chinner <david@fromorbit.com> wrote:
>> >> On Sat, Jul 12, 2014 at 06:16:54PM -0700, Samuel Just wrote:
>> >>> Hi,
>> >>>
>> >>> We are seeing reports of ceph-osd stores on xfs of files with some
>> >>> garbage data (possibly misplaced from data elsewhere in the
>> >>> filesystem).  There was a bug for a while where the ceph-osd process
>> >>> would set a value for fsx_extsize on a non-empty (possibly sparse)
>> >>> file using XFS_IOC_FSSETXATTR.  Could that plausibly result in a file
>> >>> with garbage data?
>> >>
>> >> No, setting an extent size on a non-empty file will simply fail
>> >> with EINVAL.
>>
>> AFAIR it checks whether or not any extents are actually allocated, not
>> whether the file is empty or not.
>
> FWIW, that's an implementation detail, not the definition of the
> intended behaviour of the ioctl.  Indeed, the man page says:
>
> "The fsx_xflags realtime file bit and the file's extent size may be
> changed only when the file is empty, ..."
>
> For most people, "[non-]empty file" is much more easily understood
> than "a file without real extents, but might have been written to
> and so have dirty, in-memory delayed allocation data whose
> asynchronous flushing may or may not affect the behaviour of a call
> to XFS_IOC_FSSETXATTR".
>
> i.e. the intended application behaviour is that they should only be
> able to change the extent size hint *before* any data is written to
> the file.
>
>> I think if you call fsync() or even
>> fdatasync() before close(fd), it will fail as expected.
>
> Only if you are trying to change the extent size immediately after
> the first write you do to an empty file. Which is, as per the above,
> not the recommended or intended use of the ioctl.

That's understood, but that is exactly what Sam's test program happens
to try to do, so I had to point the "file w/o real extents" thing out.

Thanks,

                Ilya

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: consequences of XFS_IOC_FSSETXATTR on non-empty file?

[Dave Chinner]

On Mon, Jul 14, 2014 at 11:24:05AM +0400, Ilya Dryomov wrote:
> On Mon, Jul 14, 2014 at 2:55 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Sun, Jul 13, 2014 at 09:01:13PM +0400, Ilya Dryomov wrote:
> >> On Sun, Jul 13, 2014 at 5:48 AM, Samuel Just <sam.just@inktank.com> wrote:
> >> I think if you call fsync() or even
> >> fdatasync() before close(fd), it will fail as expected.
> >
> > Only if you are trying to change the extent size immediately after
> > the first write you do to an empty file. Which is, as per the above,
> > not the recommended or intended use of the ioctl.
> 
> That's understood, but that is exactly what Sam's test program happens
> to try to do, so I had to point the "file w/o real extents" thing out.

Oh, I missed that there was a test program attached. That's what
happens when people top post a reply then attach text at the
bottom....

Moral of the story: don't top post ;)

Cheersm

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs