Problem doing backup of files on NTFS volume

[Tilo Schwarz]

Hi,

thank you all for the great bup backup program!

It works as expected except there is one issue I don't understand:

I have two laptops, both Debian wheezy with bup 0.25 installed.

Both Linux machines have a Windows NTFS volume (C:) which is mounted
using ntfs-3g. One on machine it is a WinXP volume, on the other a Win7
volume.

If I do "ls -R /windows/dir" on both machines, the files are shown as
expected.

If I do "bup index -uxv /some /linux /paths /windows/dir" on the machine
having the WinXP volume, it shows "/windows/dir" being indexed and a
"bup save /some /linux /paths /windows/dir" saves the files as expected.
So everything works on that machine.

One the other machine (having the Win7 volume) I basically have the same
"bup index /some /linux /paths" command as on the first machine. As long
as I do not add the windows volume to the "bup index" command, bup works
as expected (indexes and saves the linux files).

But if I add the "/windows/dir" path to the "bup index /some /linux
/paths" command, only one single file on the "/windows/dir" path is
indexed and nothing else happens. Especially the linux files, which are
usually indexed, are also not indexed. If I remove the "/windows/dir"
path from the "bup index" command again, everything is working as usual.

So, for some reason, adding the "/windows/dir" path to "bup index"
prevents "bup index" from indexing more than one file one the
"/windows/dir" path.

Maybe someone has an idea, what is going on ...

Thank you!


Tilo

[Rob Browning]

Tilo Schwarz <ma...@tilo-schwarz.de> writes:

> But if I add the "/windows/dir" path to the "bup index /some /linux
> /paths" command, only one single file on the "/windows/dir" path is
> indexed and nothing else happens. Especially the linux files, which are
> usually indexed, are also not indexed. If I remove the "/windows/dir"
> path from the "bup index" command again, everything is working as usual.
>
> So, for some reason, adding the "/windows/dir" path to "bup index"
> prevents "bup index" from indexing more than one file one the
> "/windows/dir" path.

That is odd. I assume bup index isn't printing any warnings, etc.?

Have you checked the exit code, i.e. "echo $?" afterward? I'm just
wondering if something's going wrong, but bup just isn't printing any
diagnostics.

And offhand, I'm not sure this will help, but you might want to try
cranking up the verbosity even further, like this:

BUP_DEBUG=1 bup index -vvv ...

--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Tilo Schwarz]

On 04/25/14 21:51, Rob Browning wrote:
> Tilo Schwarz <ma...@tilo-schwarz.de> writes:
>
>> But if I add the "/windows/dir" path to the "bup index /some /linux
>> /paths" command, only one single file on the "/windows/dir" path is
>> indexed and nothing else happens. Especially the linux files, which are
>> usually indexed, are also not indexed. If I remove the "/windows/dir"
>> path from the "bup index" command again, everything is working as usual.
>>
>> So, for some reason, adding the "/windows/dir" path to "bup index"
>> prevents "bup index" from indexing more than one file one the
>> "/windows/dir" path.

Thank you for your answer!

> That is odd. I assume bup index isn't printing any warnings, etc.?

No, nothing.

> Have you checked the exit code, i.e. "echo $?" afterward? I'm just
> wondering if something's going wrong, but bup just isn't printing any
> diagnostics.

Good point, I'll try this.

> And offhand, I'm not sure this will help, but you might want to try
> cranking up the verbosity even further, like this:
>
> BUP_DEBUG=1 bup index -vvv ...
>

I will try this and mail any findings.


Regards,

Tilo

[Tilo Schwarz]

On 04/25/14 21:51, Rob Browning wrote:

> Tilo Schwarz <ma...@tilo-schwarz.de> writes:
>
>> But if I add the "/windows/dir" path to the "bup index /some /linux
>> /paths" command, only one single file on the "/windows/dir" path is
>> indexed and nothing else happens. Especially the linux files, which are
>> usually indexed, are also not indexed. If I remove the "/windows/dir"
>> path from the "bup index" command again, everything is working as usual.
>>
>> So, for some reason, adding the "/windows/dir" path to "bup index"
>> prevents "bup index" from indexing more than one file one the
>> "/windows/dir" path.
>
> That is odd. I assume bup index isn't printing any warnings, etc.?
>
> Have you checked the exit code, i.e. "echo $?" afterward? I'm just
> wondering if something's going wrong, but bup just isn't printing any
> diagnostics.
>
> And offhand, I'm not sure this will help, but you might want to try
> cranking up the verbosity even further, like this:
>
> BUP_DEBUG=1 bup index -vvv ...

Today I looked closer into this:

Running the index command with "export BUP_DEBUG=2" and the windows path
"/windows/Users" added:

# bup -d $BUPDIR index -uxvvv -f $BUPDIR/latte_index
--exclude-from=/root/bup.exclude --exclude-rx=[Cc]ache /boot /etc /opt
/root /var/backups /var/log /home /windows/Users

Despite the BUP_DEBUG=2 and -vvv it prints only one line:

/windows/Users/tschwarz_2/ntuser.ini

I checked the exit code:

# echo $?
245

So something is going wrong. If I remove the path "/windows/Users" all
other files are indexed as usual and the exit code is 0.



During the testing above something else happened, which seems strange to me:

I tried to redirect stdin and stderr to a file using &> :

# bup -d $BUPDIR index -uxvvv -f $BUPDIR/latte_index
--exclude-from=/root/bup.exclude --exclude-rx=[Cc]ache /boot /etc /opt
/root /var/backups /var/log /home /windows/Users &> /media/usb/bup.log
Segmentation fault

If I do the redirection, I get a Segmentation fault, if I remove the
redirection, the Segmentation fault goes away.


Thanks a lot!

Tilo

[Rob Browning]

Tilo Schwarz <ma...@tilo-schwarz.de> writes:

> During the testing above something else happened, which seems strange to me:
>
> I tried to redirect stdin and stderr to a file using &> :
>
> # bup -d $BUPDIR index -uxvvv -f $BUPDIR/latte_index
> --exclude-from=/root/bup.exclude --exclude-rx=[Cc]ache /boot /etc /opt
> /root /var/backups /var/log /home /windows/Users &> /media/usb/bup.log
> Segmentation fault
>
> If I do the redirection, I get a Segmentation fault, if I remove the
> redirection, the Segmentation fault goes away.

And does this also depend on including /windows/Users?

[Rob Browning]

Tilo Schwarz <ma...@tilo-schwarz.de> writes:

> Despite the BUP_DEBUG=2 and -vvv it prints only one line:
>
> /windows/Users/tschwarz_2/ntuser.ini
>
> I checked the exit code:
>
> # echo $?
> 245

You could also try "strace -f", but at this point, if you're interested,
I'd probably just start adding

print >> sys.stderr, "HERE"

statements to cmd-index.py until I figured out which statement was
causing bup to exit.

Or if you want to be fancier:

from inspect import getframeinfo
import os.path

def dbg(items, frame, out=sys.stderr):
info = getframeinfo(frame)
filename = os.path.basename(info.filename)
print >> out, '%s:%d ' % (filename, info.lineno),
for item in items:
print >> out, item,
out.write('\n')
out.flush()

and then:

dbg('whatever')

wherever you want to print the file and line number.

Hope this helps.

[Tilo Schwarz]

On 05/05/14 02:18, Rob Browning wrote:
> Tilo Schwarz <ma...@tilo-schwarz.de> writes:
>
>> Despite the BUP_DEBUG=2 and -vvv it prints only one line:
>>
>> /windows/Users/tschwarz_2/ntuser.ini
>>
>> I checked the exit code:
>>
>> # echo $?
>> 245
>
> You could also try "strace -f", but at this point, if you're interested,

Hi,

so I tried strace. Below is the strace output of the failing process and
the mount options of the windows file system. It fails right after an
ioctl call (ENOSYS):

> tail -20 /media/usb/bup_strace.4936
munmap(0x7f94bd3c8000, 1238) = 0
close(8) = 0
socket(PF_FILE, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0) = 8
connect(8, {sa_family=AF_FILE, path="/var/run/nscd/socket"}, 110) = -1
ENOENT (No such file or directory)
close(8) = 0
socket(PF_FILE, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0) = 8
connect(8, {sa_family=AF_FILE, path="/var/run/nscd/socket"}, 110) = -1
ENOENT (No such file or directory)
close(8) = 0
open("/etc/group", O_RDONLY|O_CLOEXEC) = 8
lseek(8, 0, SEEK_CUR) = 0
fstat(8, {st_mode=S_IFREG|0644, st_size=719, ...}) = 0
mmap(NULL, 719, PROT_READ, MAP_SHARED, 8, 0) = 0x7f94bd3c8000
lseek(8, 719, SEEK_SET) = 719
munmap(0x7f94bd3c8000, 719) = 0
close(8) = 0
getxattr("/windows/Users/tschwarz_2/ntuser.ini",
"system.posix_acl_access", 0x0, 0) = -1 EOPNOTSUPP (Operation not supported)
open("/windows/Users/tschwarz_2/ntuser.ini",
O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_NOATIME) = 8
ioctl(8, FS_IOC32_GETFLAGS or FS_IOC_GETFLAGS, 0x7fffad567f74) = -1
ENOSYS (Function not implemented)
close(8) = 0
--- SIGSEGV (Segmentation fault) @ 0 (0) ---

> grep windows /media/usb/mount.txt
/dev/sdb3 on /windows type fuseblk
(rw,nosuid,nodev,relatime,user_id=0,group_id=0,allow_other,blksize=4096)

I try the debugging using print next time.

Thank you!

Tilo

[Tilo Schwarz]

On 05/05/14 02:18, Rob Browning wrote:

> Tilo Schwarz <ma...@tilo-schwarz.de> writes:
>
>> Despite the BUP_DEBUG=2 and -vvv it prints only one line:
>>
>> /windows/Users/tschwarz_2/ntuser.ini
>>
>> I checked the exit code:
>>
>> # echo $?
>> 245
>
> You could also try "strace -f", but at this point, if you're interested,
> I'd probably just start adding
>
> print >> sys.stderr, "HERE"
>
> statements to cmd-index.py until I figured out which statement was
> causing bup to exit.

Hi,

I looked deeper into this using print util I found the offending line.

bup index dies in "_add_linux_attr()" marked below in
/usr/lib/bup/bup/metadata.py

def from_path(path, statinfo=None, archive_path=None,
save_symlinks=True, hardlink_target=None):
result = Metadata()
result.path = archive_path
st = statinfo or xstat.lstat(path)
result.size = st.st_size
result._add_common(path, st)
if save_symlinks:
result._add_symlink_target(path, st)
result._add_hardlink_target(hardlink_target)
result._add_posix1e_acl(path, st)
result._add_linux_attr(path, st) <-- dies here
result._add_linux_xattr(path, st)
return result

When I unconditionally set

get_linux_file_attr = set_linux_file_attr = None

at the top of /usr/lib/bup/bup/metadata.py

the indexing of the windows directory runs as normally and no errors are
emitted (_add_linux_attr does nothing in that case).

So for some reason bup beliefs, the windows directory is a linux
directory and in _add_linux_attr the indexing fails.

What I don't understand: The two machines which I use have the same
Debian wheezy system with the same bup. On the one machine the Windows
XP directory is indexed correctly, but on the other machine the Windows
7 directory indexing fails.

Thank you!

Tilo

> Or if you want to be fancier:
>
> from inspect import getframeinfo
> import os.path
>
> def dbg(items, frame, out=sys.stderr):
> info = getframeinfo(frame)
> filename = os.path.basename(info.filename)
> print >> out, '%s:%d ' % (filename, info.lineno),
> for item in items:
> print >> out, item,
> out.write('\n')
> out.flush()
>
> and then:
>
> dbg('whatever')
>
> wherever you want to print the file and line number.
>
> Hope this helps.
>


--

Viele Grüße,

Tilo

[Rob Browning]

Tilo Schwarz <ma...@tilo-schwarz.de> writes:

> I looked deeper into this using print util I found the offending line.
>
> bup index dies in "_add_linux_attr()" marked below in
> /usr/lib/bup/bup/metadata.py

Actually, if the strace you posted earlier corresponds to a matching
run, then I suspect bup at least makes it to _add_linux_xattr() because
the strace does (I suspect it's the GETFLAGS call in _helpers.c that we
see below):

getxattr("/windows/Users/tschwarz_2/ntuser.ini",
"system.posix_acl_access", 0x0, 0) = -1 EOPNOTSUPP (Operation not supported)
open("/windows/Users/tschwarz_2/ntuser.ini",
O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_NOATIME) = 8
ioctl(8, FS_IOC32_GETFLAGS or FS_IOC_GETFLAGS, 0x7fffad567f74) = -1
ENOSYS (Function not implemented)
close(8) = 0
--- SIGSEGV (Segmentation fault) @ 0 (0) ---

Regardless, bup shouldn't segfault.

If you're comfortable making some changes on the C side, it might be
interesting to try adding a few lines like this in
bup_get_linux_file_attr() (after the getflags call), so we can see
exactly where it fails:

fprint(stderr, "%s:%d", __FILE__, __LINE__);

You might also want to add a

print >> sys.stderr, path

just before the call to get_linux_file_attr() in metadata.py so we can
see if the path looks ok. And another similar print statement after it,
so we can see if bup ever returns from the call.

And note, that after every change to one of the .c files, you'll need to
re-run "make" for it to have any effect on bup.

> So for some reason bup beliefs, the windows directory is a linux
> directory and in _add_linux_attr the indexing fails.

Bup doesn't know anything about the type of the filesystem in that
sense, it just tries the attr call and moves on if it fails.

> What I don't understand: The two machines which I use have the same
> Debian wheezy system with the same bup. On the one machine the Windows
> XP directory is indexed correctly, but on the other machine the Windows
> 7 directory indexing fails.

Any other differences between the systems (i.e. kernel, python module
versions, python versions, architectures (say i386 vs amd64), etc.)?

[Tilo Schwarz]

On 05/28/14 07:02, Rob Browning wrote:
> Tilo Schwarz <ma...@tilo-schwarz.de> writes:
>
>> I looked deeper into this using print util I found the offending line.
>>
>> bup index dies in "_add_linux_attr()" marked below in
>> /usr/lib/bup/bup/metadata.py
>
> Actually, if the strace you posted earlier corresponds to a matching
> run, then I suspect bup at least makes it to _add_linux_xattr() because
> the strace does (I suspect it's the GETFLAGS call in _helpers.c that we
> see below):

You're right of course, that's what I meant: bup dies after entering
_add_linux_xattr().

> If you're comfortable making some changes on the C side, it might be
> interesting to try adding a few lines like this in
> bup_get_linux_file_attr() (after the getflags call), so we can see
> exactly where it fails:
>
> fprint(stderr, "%s:%d", __FILE__, __LINE__);
>
> You might also want to add a
>
> print >> sys.stderr, path
>
> just before the call to get_linux_file_attr() in metadata.py so we can
> see if the path looks ok. And another similar print statement after it,
> so we can see if bup ever returns from the call.

I'll have a look.

> And note, that after every change to one of the .c files, you'll need to
> re-run "make" for it to have any effect on bup.
>
>> So for some reason bup beliefs, the windows directory is a linux
>> directory and in _add_linux_attr the indexing fails.
>
> Bup doesn't know anything about the type of the filesystem in that
> sense, it just tries the attr call and moves on if it fails.

Ok.

>> What I don't understand: The two machines which I use have the same
>> Debian wheezy system with the same bup. On the one machine the Windows
>> XP directory is indexed correctly, but on the other machine the Windows
>> 7 directory indexing fails.
>
> Any other differences between the systems (i.e. kernel, python module
> versions, python versions, architectures (say i386 vs amd64), etc.)?

Actually yes: The "Ok-Machine" is

Linux 3.2.0-4-686-pae #1 SMP Debian 3.2.57-3 i686 GNU/Linux

The "Bad-Machine" is amd64 and a much younger kernel (I have to look it up).

Regards,

Tilo

[dar...@arcor.de]

I'm experiencing the same issue while trying to back up from an NTFS partition mounted using ntfs-3g.

BUP_DEBUG=1 bup index -vvv

prints between one and five filenames before exiting with code 245. I get this same error with three different harddrives. Indexing ext4 folders works fine as far as I can tell.

[Tilo Schwarz]

On 05/28/14 07:02, Rob Browning wrote:

> Tilo Schwarz <ma...@tilo-schwarz.de> writes:
>
>> I looked deeper into this using print util I found the offending line.
>>
>> bup index dies in "_add_linux_attr()" marked below in
>> /usr/lib/bup/bup/metadata.py
>
> Actually, if the strace you posted earlier corresponds to a matching
> run, then I suspect bup at least makes it to _add_linux_xattr() because
> the strace does (I suspect it's the GETFLAGS call in _helpers.c that we
> see below):
>
> getxattr("/windows/Users/tschwarz_2/ntuser.ini",
> "system.posix_acl_access", 0x0, 0) = -1 EOPNOTSUPP (Operation not supported)
> open("/windows/Users/tschwarz_2/ntuser.ini",
> O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_NOATIME) = 8
> ioctl(8, FS_IOC32_GETFLAGS or FS_IOC_GETFLAGS, 0x7fffad567f74) = -1
> ENOSYS (Function not implemented)
> close(8) = 0
> --- SIGSEGV (Segmentation fault) @ 0 (0) ---
>
> Regardless, bup shouldn't segfault.
>
> If you're comfortable making some changes on the C side, it might be
> interesting to try adding a few lines like this in
> bup_get_linux_file_attr() (after the getflags call), so we can see
> exactly where it fails:
>
> fprint(stderr, "%s:%d", __FILE__, __LINE__);

I couldn't make the C-changes yet (because of build-deps etc.). But I
checked using "print markers" like shown below, that bup enters
get_linux_file_attr(path) but does not return. Sadly I forgot to print
the path - next time.

556 def _add_linux_attr(self, path, st):
557 if not get_linux_file_attr: return
558 if stat.S_ISREG(st.st_mode) or stat.S_ISDIR(st.st_mode):
559 try:
print 'marker 1'
sys.stdout.flush()
560 attr = get_linux_file_attr(path)
print 'marker 2'
sys.stdout.flush()
561 if attr != 0:
562 self.linux_attr = attr
563 except OSError, e:
564 if e.errno == errno.EACCES:
565 add_error('read Linux attr: %s' % e)
566 elif e.errno in (errno.ENOTTY, errno.ENOSYS,
errno.EOPNOTSUPP):
567 # Assume filesystem doesn't support attrs.
568 return
569 else:
570 raise

Best Regards,
Tilo

[Rob Browning]

OK, well if one of you can get to the point of being able to build bup,
and has time to test, we should be able to figure out where/why it's
dying on the C side. I'll probably go take another look at the code
too, when I get a minute.

Thanks

[dar...@arcor.de]

Ironically, it seems that this

return PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);

line is causing the crash. The error handling in metadata.py is never executed. Replacing it with

return Py_BuildValue("I", 0);

has allowed me to make a successful backup, but of course that's not a real solution. I'm not familiar with Python, so I don't know why this would crash.

[Rob Browning]

If you can build bup then it'd be interesting to see what this prints,
when placed just before the return:

fprintf(stderr, "%s:%d set-from-errno-info %d %p\n",
__FILE__, __LINE__, errno, path);
fflush(stderr);

I'd also make sure (if you haven't already) to place some python-side
print statements right after the offending call (in both the exceptional
and non-exceptional paths), so we can make sure we're right about
exactly which line causes the trouble. You might want a flush there
too, i.e.:

print >> sys.stderr, ...
sys.stderr.flush()

[dar...@arcor.de]

On Saturday, June 28, 2014 10:36:01 PM UTC+2, Rob Browning wrote:

dar...@arcor.de writes:

> Ironically, it seems that this
>
> return PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);
>
> line is causing the crash. The error handling in metadata.py is never
> executed. Replacing it with
>
> return Py_BuildValue("I", 0);
>
> has allowed me to make a successful backup, but of course that's not a real
> solution. I'm not familiar with Python, so I don't know why this would
> crash.

If you can build bup then it'd be interesting to see what this prints,
when placed just before the return:

  fprintf(stderr, "%s:%d set-from-errno-info %d %p\n",
          __FILE__, __LINE__, errno, path);
  fflush(stderr);

This prints

_helpers.c:836 set-from-errno-info 38 0x7ff400000000

I'd also make sure (if you haven't already) to place some python-side
print statements right after the offending call (in both the exceptional
and non-exceptional paths), so we can make sure we're right about
exactly which line causes the trouble.

I had already done this, but I added one more print on the c++ side between call and return. The call to  PyErr_SetFromErrnoWithFilename never returns.

[Rob Browning]

dar...@arcor.de writes:

> On Saturday, June 28, 2014 10:36:01 PM UTC+2, Rob Browning wrote:

>> If you can build bup then it'd be interesting to see what this prints,
>> when placed just before the return:
>>
>> fprintf(stderr, "%s:%d set-from-errno-info %d %p\n",
>> __FILE__, __LINE__, errno, path);
>> fflush(stderr);
>>
>> This prints
> _helpers.c:836 set-from-errno-info 38 0x7ff400000000

OK, and how about this?

fprintf(stderr, "%s:%d set-from-errno-info %d %p %s\n",
__FILE__, __LINE__, errno, path, path);
fflush(stderr);

If that succeeds (i.e. if path isn't somehow broken), then I'm not sure
what's going on -- unless SetFromErrnoWithFilename() is just broken on
your system, or something earlier has put the program in an untenable
state.

I suppose you could try running it under gdb.

[dar...@arcor.de]

On Monday, June 30, 2014 3:37:55 AM UTC+2, Rob Browning wrote:

OK, and how about this?

    fprintf(stderr, "%s:%d set-from-errno-info %d %p %s\n",
            __FILE__, __LINE__, errno, path, path);
    fflush(stderr);

If that succeeds (i.e. if path isn't somehow broken), then I'm not sure
what's going on -- unless SetFromErrnoWithFilename() is just broken on
your system, or something earlier has put the program in an untenable
state.

I suppose you could try running it under gdb.

This indeed breaks (I thought that that pointer looked very suspicious...). Interestingly, when I add
 
        volatile int i = 0;
        while (i == 0)
        {
            usleep(100000);
        }

in bup_get_linux_file_attr to give myself time to attach gdb, the probem disappears, path takes on a sane value and the indexing proceeds. Changing the condition so the sleep is never executed makes it work even without the debugger attached. I don't know exactly how or why the presence of a volatile variable affects the rest of the function, but I think it may be a hint that something subtle is going on...

[Rob Browning]

dar...@arcor.de writes:

> This indeed breaks (I thought that that pointer looked very suspicious...).
> Interestingly, when I add
>
> volatile int i = 0;
> while (i == 0)
> {
> usleep(100000);
> }
>
> in bup_get_linux_file_attr to give myself time to attach gdb, the probem
> disappears, path takes on a sane value and the indexing proceeds.

You could also try running bup like this:

gdb -ex r --args python bup ...

which should allow you to try the unmodified code.

> Changing the condition so the sleep is never executed makes it work
> even without the debugger attached. I don't know exactly how or why
> the presence of a volatile variable affects the rest of the function,
> but I think it may be a hint that something subtle is going on...

Since it looks like path is the problem (or at least the symptom) --
it'd be interesting to see *when* it becomes troublesome.

Assuming the debug instrumentation doesn't eliminate the crash, you
could probably either watch path via gdb, or print the path value
throughout the function.

[dar...@arcor.de]

On Monday, June 30, 2014 7:46:39 PM UTC+2, Rob Browning wrote:

Since it looks like path is the problem (or at least the symptom) --
it'd be interesting to see *when* it becomes troublesome.

This

rc = ioctl(fd, FS_IOC_GETFLAGS, &attr);

is the line that messes up the path variable.

[Rob Browning]

dar...@arcor.de writes:

> rc = ioctl(fd, FS_IOC_GETFLAGS, &attr);
> is the line that messes up the path variable.

By messes up, do you mean the pointer changes?

And is there anything that might be particularly unusual about your
system (say with respect to architecture, compiler, kernel, headers,
etc.)?

Perhaps this earlier discussion is relevant?

http://thread.gmane.org/gmane.linux.file-systems/80164

What happens if you change attr to "unsigned long"?

[dar...@arcor.de]

On Monday, June 30, 2014 9:24:37 PM UTC+2, Rob Browning wrote:

By messes up, do you mean the pointer changes?

Yes

And is there anything that might be particularly unusual about your
system (say with respect to architecture, compiler, kernel, headers,
etc.)?

Not that I know of, I'm running Linux 3.15.1-1-ARCH x86_64

Perhaps this earlier discussion is relevant?

  http://thread.gmane.org/gmane.linux.file-systems/80164

It seems extremely relevant!

What happens if you change attr to "unsigned long"?

That fixes it!
 

[Rob Browning]

dar...@arcor.de writes:

> That fixes it!

Great -- so what kind of system are you on? (i.e. architecture, etc.)

[dar...@arcor.de]

On Monday, June 30, 2014 11:25:57 PM UTC+2, Rob Browning wrote:

Great -- so what kind of system are you on?  (i.e. architecture, etc.)

Kernel        : Linux 3.15.1-1-ARCH (x86_64)
Compiled        : #1 SMP PREEMPT Tue Jun 17 09:32:20 CEST 2014
C Library        : GNU C Library version 2.19 (stable)
Default C Compiler        : GNU C Compiler version 4.9.0 20140604 (prerelease) (GCC)
Distribution        : Arch Linux
Processor        : 4x Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz

Using this bup package: https://aur.archlinux.org/packages/bup/

If there's anything else you need, let me know.

[dar...@arcor.de]

Any word on getting a fix into production?

[Rob Browning]

dar...@arcor.de writes:

> Any word on getting a fix into production?

First we'll have to figure out what the fix would be. My impression
(from the Linux filesystem thread) is that int is the correct attr type.

So we'll either have to have some way to detect that the system is
behaving incorrectly, or the system in question will have to be fixed.

...assuming that I didn't misunderstand the situation the last time I
delved.

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> ...assuming that I didn't misunderstand the situation the last time I
> delved.

And to elaborate a bit, my understanding from (for example) here:

http://article.gmane.org/gmane.linux.file-systems/80223

is that the correct type is int. If that's right, then the first
question is, why does your system appear to be using a long?

And given that we haven't seen other reports, and that Linux/amd64 is
probably our most common platform right now, I suspect that either int
is typical, or that elsewhere the problem is normally masked somehow.

Thanks

[dar...@arcor.de]

According to this article
http://lwn.net/Articles/575846/
int is indeed the correct type. The problem seems to be that FUSE writes a long. I can confirm that other FUSE filesystems (fuseiso) produce the same crash, while the kernel mode read-only NTFS driver doesn't.
Unfortunately, the FUSE developers seem to have decided that everyone else is wrong
https://lkml.org/lkml/2014/1/6/917
so the proposed fix never got implemented.

[Rob Browning]

dar...@arcor.de writes:

> http://lwn.net/Articles/575846/
> int is indeed the correct type. The problem seems to be that FUSE writes a
> long.

OK, now that rings a bell.

> I can confirm that other FUSE filesystems (fuseiso) produce the same
> crash, while the kernel mode read-only NTFS driver doesn't.
> Unfortunately, the FUSE developers seem to have decided that everyone else
> is wrong
> https://lkml.org/lkml/2014/1/6/917
> so the proposed fix never got implemented.

Indeed a mess. Suggestions?

Theoretically, we could try to determine which fds are on FUSE mounted
filesystems and skip the IOC calls there, but that would definitely
require some work (perhaps a /proc/self/mountinfo parser, etc.). And to
make it efficient, we might need to cache the dev_t -> fs mapping, which
of course means we're vulnerable to mount point changes during a run
(..."don't do that then"?).

A simpler approach would be to add an option to disable support for
Linux file attrs (which may be something we want anyway), but then
everyone affected has to know they need to use it, and it's a blunt
instrument.

As a *hack*, you might try to just detect little-endian systems, and use
long there across the board, but I think that would still leave a 64-bit
big-endian systems completely broken -- which at least for Debian would
currently include s390x (and ppc64).

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> As a *hack*, you might try to just detect little-endian systems, and use
> long there across the board, but I think that would still leave a 64-bit
> big-endian systems completely broken -- which at least for Debian would
> currently include s390x (and ppc64).

Hmm. Short-term, that might at least be better than what we have now.

i.e. we'd switch back to long, and disable Linux attrs on big-endian
systems where sizeof(long) > sizeof(int). Then we'd document the issue
as clearly as we can for those affected, until it's either fixed
upstream, or we have time to implement something more sophisticated.

Thoughts?

[dar...@arcor.de]

On Monday, July 28, 2014 7:45:46 PM UTC+2, Rob Browning wrote:

Rob Browning <r...@defaultvalue.org> writes:

> As a *hack*, you might try to just detect little-endian systems, and use
> long there across the board, but I think that would still leave a 64-bit
> big-endian systems completely broken -- which at least for Debian would
> currently include s390x (and ppc64).

Hmm.  Short-term, that might at least be better than what we have now.

I agree. If I understand the issue correctly, the long term solution should really be a fix in FUSE (although I guess I can see the argument that everyone else is doing it wrong, I think having a working system is more important here), but in the short term this seems like a good solution.
An option for disabling Linux file attr support altogether might also be a good idea.
 

[Rob Browning]

dar...@arcor.de writes:

> I agree. If I understand the issue correctly, the long term solution should
> really be a fix in FUSE (although I guess I can see the argument that
> everyone else is doing it wrong, I think having a working system is more
> important here), but in the short term this seems like a good solution.
> An option for disabling Linux file attr support altogether might also be a
> good idea.

Perhaps an improved version of something like the rough patch below?

It'd need more documentation, and I suppose we could restructure it to
include a --no-really-fuse-isnt-involved override option with loud
warnings for any big-endian users, but that would reintroduce the crash
risk.

[Rob Browning]

Use long for the Linux attr type, so that it should work on all
little-endian systems, across both normal and FUSE-backed filesystems.

Disable Linux attr support for now on big-endian systems where
sizeof(long) > sizeof(int). See the changes to bup-index.md and
_helpers.c for more information.

Eventually, we may want to add an argument to allow re-enabling
support on affected systems, when the user can guarantee a homogeneous
filesystem type -- or better yet, the kernel and FUSE will finally
sort out their issues.

Thanks to Tilo Schwarz <ma...@tilo-schwarz.de> and dar...@arcor.de for
reporting and pursuing the problems that lead to this patch.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
---

Proposed for master.

Documentation/bup-index.md | 9 +++++++++
lib/bup/_helpers.c | 24 +++++++++++++++++++-----
lib/bup/metadata.py | 18 ++++++++++++++++--
3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/Documentation/bup-index.md b/Documentation/bup-index.md
index c69c2ab..280d9eb 100644
--- a/Documentation/bup-index.md
+++ b/Documentation/bup-index.md
@@ -42,6 +42,15 @@ need the same information).

# NOTES

+At the moment, bup will ignore Linux attributes (cf. chattr(1) and
+lsattr(1)) on some systems (any big-endian systems where sizeof(long)
+< sizeof(int)). This is because the Linux kernel and FUSE currently
+disagree over the type of the attr system call arguments, and so on
+big-endian systems there's no way to get the results without the risk
+of stack corruption (http://lwn.net/Articles/575846/). In these
+situations, bup will print a warning the first time Linux attrs are
+relevant during any index/save/restore operation.
+
bup makes accommodations for the expected "worst-case" filesystem
timestamp resolution -- currently one second; examples include VFAT,
ext2, ext3, small ext4, etc. Since bup cannot know the filesystem
diff --git a/lib/bup/_helpers.c b/lib/bup/_helpers.c
index 6957d2c..c0ff61e 100644
--- a/lib/bup/_helpers.c
+++ b/lib/bup/_helpers.c
@@ -811,11 +811,21 @@ static PyObject *fadvise_done(PyObject *self, PyObject *args)
}


+// Currently the Linux kernel and FUSE disagree over the type for
+// FS_IOC_GETFLAGS and FS_IOC_SETFLAGS. The kernel actually uses int,
+// but FUSE chose long (matching the declaration in linux/fs.h). So
+// if you use int, and then traverse a FUSE filesystem, you may
+// corrupt the stack. But if you use long, then you may get invalid
+// results on big-endian systems.
+//
+// For now, we just use long, and then disable Linux attrs entirely
+// (with a warning) in helpers.py on systems that are affected.
+
#ifdef BUP_HAVE_FILE_ATTRS
static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)
{
int rc;
- unsigned int attr;
+ unsigned long attr;
char *path;
int fd;

@@ -826,8 +836,9 @@ static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)
if (fd == -1)
return PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);

- attr = 0;
+ attr = 0; // Handle int/long mismatch (see above)

rc = ioctl(fd, FS_IOC_GETFLAGS, &attr);

+ assert(attr <= UINT_MAX); // Kernel type is actually int
if (rc == -1)
{
close(fd);
@@ -835,7 +846,7 @@ static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)
}

close(fd);
- return Py_BuildValue("I", attr);
+ return PyLong_FromUnsignedLong(attr);
}
#endif /* def BUP_HAVE_FILE_ATTRS */

@@ -845,7 +856,8 @@ static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)
static PyObject *bup_set_linux_file_attr(PyObject *self, PyObject *args)
{
int rc;
- unsigned int orig_attr, attr;
+ unsigned long orig_attr;
+ unsigned int attr;
char *path;
PyObject *py_attr;
int fd;
@@ -869,13 +881,15 @@ static PyObject *bup_set_linux_file_attr(PyObject *self, PyObject *args)
| FS_TOPDIR_FL | FS_NOCOW_FL;

// The extents flag can't be removed, so don't (see chattr(1) and chattr.c).
+ orig_attr = 0; // Handle int/long mismatch (see above)
rc = ioctl(fd, FS_IOC_GETFLAGS, &orig_attr);
+ assert(orig_attr <= UINT_MAX); // Kernel type is actually int
if (rc == -1)
{
close(fd);
return PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);
}
- attr |= (orig_attr & FS_EXTENT_FL);
+ attr |= ((unsigned int) orig_attr) & FS_EXTENT_FL;

rc = ioctl(fd, FS_IOC_SETFLAGS, &attr);
if (rc == -1)
diff --git a/lib/bup/metadata.py b/lib/bup/metadata.py
index b4cedf3..86eabb5 100644
--- a/lib/bup/metadata.py
+++ b/lib/bup/metadata.py
@@ -4,7 +4,7 @@
#
# This code is covered under the terms of the GNU Library General
# Public License as described in the bup LICENSE file.
-import errno, os, sys, stat, time, pwd, grp, socket
+import errno, os, sys, stat, time, pwd, grp, socket, struct
from cStringIO import StringIO
from bup import vint, xstat
from bup.drecurse import recursive_dirlist
@@ -42,7 +42,19 @@ except ImportError:
# not on Linux, in which case files don't have any linux attrs anyway, so
# lacking the functions isn't a problem.

get_linux_file_attr = set_linux_file_attr = None

-
+
+
+_suppress_linux_file_attr = \
+ sys.byteorder == 'big' and struct.calcsize('=l') > struct.calcsize('=i')
+
+def check_linux_file_attr_api():
+ global get_linux_file_attr, set_linux_file_attr
+ if not (get_linux_file_attr and set_linux_file_attr):
+ return
+ if _suppress_linux_file_attr:
+ log('Warning: Linux attr support disabled (see "bup help index").\n')
+ get_linux_file_attr = set_linux_file_attr = None
+

# WARNING: the metadata encoding is *not* stable yet. Caveat emptor!

@@ -554,6 +566,7 @@ class Metadata:
## Linux attributes (lsattr(1), chattr(1))

def _add_linux_attr(self, path, st):
+ check_linux_file_attr_api()
if not get_linux_file_attr: return
if stat.S_ISREG(st.st_mode) or stat.S_ISDIR(st.st_mode):
try:
@@ -585,6 +598,7 @@ class Metadata:

def _apply_linux_attr_rec(self, path, restore_numeric_ids=False):
if self.linux_attr:
+ check_linux_file_attr_api()
if not set_linux_file_attr:
add_error("%s: can't restore linuxattrs: "
"linuxattr support missing.\n" % path)
--
2.0.1

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> Use long for the Linux attr type, so that it should work on all
> little-endian systems, across both normal and FUSE-backed filesystems.

Oh, and if/when you can, could you test this on the affected systems?

Thanks

[Tilo Schwarz]

On 07/30/14 18:24, Rob Browning wrote:
> Rob Browning <r...@defaultvalue.org> writes:
>
>> Use long for the Linux attr type, so that it should work on all
>> little-endian systems, across both normal and FUSE-backed filesystems.
>
> Oh, and if/when you can, could you test this on the affected systems?

It will take a while until I access that system again, but I will post
any findings I have.

Thank you for chasing this down and finding a solution!


Regards,

Tilo

[Rob Browning]

Use long for the Linux attr type, so that it should work on all
little-endian systems, across both normal and FUSE-backed filesystems.

Disable Linux attr support for now on big-endian systems where
sizeof(long) > sizeof(int). See the changes to bup-index.md and
_helpers.c for more information.

Eventually, we may want to add an argument to allow re-enabling
support on affected systems, when the user can guarantee a homogeneous
filesystem type -- or better yet, the kernel and FUSE will finally
sort out their issues.

Thanks to Tilo Schwarz <ma...@tilo-schwarz.de> and dar...@arcor.de for
reporting and pursuing the problems that lead to this patch.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
---

Pushed to master.

This is identical to the previous version except for a change to the
check_linux_file_attr_api() logic. i.e.:

def check_linux_file_attr_api():
global get_linux_file_attr, set_linux_file_attr
- if not (get_linux_file_attr and set_linux_file_attr):
+ if not (get_linux_file_attr or set_linux_file_attr):
return
if _suppress_linux_file_attr:

log('Warning: Linux attr support disabled (see "bup help index").\n')

Documentation/bup-index.md | 9 +++++++++
lib/bup/_helpers.c | 24 +++++++++++++++++++-----
lib/bup/metadata.py | 18 ++++++++++++++++--
3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/Documentation/bup-index.md b/Documentation/bup-index.md
index c69c2ab..280d9eb 100644
--- a/Documentation/bup-index.md
+++ b/Documentation/bup-index.md
@@ -42,6 +42,15 @@ need the same information).

# NOTES

+At the moment, bup will ignore Linux attributes (cf. chattr(1) and
+lsattr(1)) on some systems (any big-endian systems where sizeof(long)
+< sizeof(int)). This is because the Linux kernel and FUSE currently
+disagree over the type of the attr system call arguments, and so on
+big-endian systems there's no way to get the results without the risk
+of stack corruption (http://lwn.net/Articles/575846/). In these
+situations, bup will print a warning the first time Linux attrs are
+relevant during any index/save/restore operation.
+
bup makes accommodations for the expected "worst-case" filesystem
timestamp resolution -- currently one second; examples include VFAT,
ext2, ext3, small ext4, etc. Since bup cannot know the filesystem
diff --git a/lib/bup/_helpers.c b/lib/bup/_helpers.c

index 4d2f9fd..65a8b6b 100644
--- a/lib/bup/_helpers.c
+++ b/lib/bup/_helpers.c
@@ -815,11 +815,21 @@ static PyObject *fadvise_done(PyObject *self, PyObject *args)

}


+// Currently the Linux kernel and FUSE disagree over the type for
+// FS_IOC_GETFLAGS and FS_IOC_SETFLAGS. The kernel actually uses int,
+// but FUSE chose long (matching the declaration in linux/fs.h). So
+// if you use int, and then traverse a FUSE filesystem, you may
+// corrupt the stack. But if you use long, then you may get invalid
+// results on big-endian systems.
+//
+// For now, we just use long, and then disable Linux attrs entirely
+// (with a warning) in helpers.py on systems that are affected.
+
#ifdef BUP_HAVE_FILE_ATTRS
static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)
{
int rc;
- unsigned int attr;
+ unsigned long attr;
char *path;
int fd;

@@ -830,8 +840,9 @@ static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)

if (fd == -1)
return PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);

- attr = 0;
+ attr = 0; // Handle int/long mismatch (see above)
rc = ioctl(fd, FS_IOC_GETFLAGS, &attr);
+ assert(attr <= UINT_MAX); // Kernel type is actually int
if (rc == -1)
{
close(fd);

@@ -839,7 +850,7 @@ static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)

}

close(fd);
- return Py_BuildValue("I", attr);
+ return PyLong_FromUnsignedLong(attr);
}
#endif /* def BUP_HAVE_FILE_ATTRS */

@@ -849,7 +860,8 @@ static PyObject *bup_get_linux_file_attr(PyObject *self, PyObject *args)

static PyObject *bup_set_linux_file_attr(PyObject *self, PyObject *args)
{
int rc;
- unsigned int orig_attr, attr;
+ unsigned long orig_attr;
+ unsigned int attr;
char *path;
PyObject *py_attr;
int fd;

@@ -873,13 +885,15 @@ static PyObject *bup_set_linux_file_attr(PyObject *self, PyObject *args)

| FS_TOPDIR_FL | FS_NOCOW_FL;

// The extents flag can't be removed, so don't (see chattr(1) and chattr.c).
+ orig_attr = 0; // Handle int/long mismatch (see above)
rc = ioctl(fd, FS_IOC_GETFLAGS, &orig_attr);
+ assert(orig_attr <= UINT_MAX); // Kernel type is actually int
if (rc == -1)
{
close(fd);
return PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);
}
- attr |= (orig_attr & FS_EXTENT_FL);
+ attr |= ((unsigned int) orig_attr) & FS_EXTENT_FL;

rc = ioctl(fd, FS_IOC_SETFLAGS, &attr);
if (rc == -1)
diff --git a/lib/bup/metadata.py b/lib/bup/metadata.py

index b4cedf3..685c663 100644

--- a/lib/bup/metadata.py
+++ b/lib/bup/metadata.py
@@ -4,7 +4,7 @@
#
# This code is covered under the terms of the GNU Library General
# Public License as described in the bup LICENSE file.
-import errno, os, sys, stat, time, pwd, grp, socket
+import errno, os, sys, stat, time, pwd, grp, socket, struct
from cStringIO import StringIO
from bup import vint, xstat
from bup.drecurse import recursive_dirlist
@@ -42,7 +42,19 @@ except ImportError:
# not on Linux, in which case files don't have any linux attrs anyway, so
# lacking the functions isn't a problem.
get_linux_file_attr = set_linux_file_attr = None
-
+
+
+_suppress_linux_file_attr = \
+ sys.byteorder == 'big' and struct.calcsize('=l') > struct.calcsize('=i')
+
+def check_linux_file_attr_api():
+ global get_linux_file_attr, set_linux_file_attr

+ if not (get_linux_file_attr or set_linux_file_attr):