[Toybox] [PATCH] getfattr.c: fix overlayfs merged dir stuck

[Weizhao Ouyang]

On 2022/9/1 05:29, Rob Landley wrote:
> On 8/31/22 12:00, Weizhao Ouyang wrote:
>> On 2022/8/31 18:48, Rob Landley wrote:
>>> On 8/31/22 05:35, Rob Landley wrote:
>>>> On 8/30/22 10:31, Weizhao Ouyang wrote:
>>>>> When getfattx reading overlayfs merged dirs, listxattr will got
>>>>> different keys_len with zero size and determined size, then it will
>>>>> stuck in this while scope. Update the keys_len after the second
>>>>> listxattr to fix it.
>>>> Do you have a test case I can use to reproduce this? (I've never used xattrs
>>>> together with overlayfs before.)
>> Hi Rob, here is a simple testcase (with overlayfs module):
>> mkdir lower upper merged work
>> mkdir lower/dir upper/dir
>> sudo mount -t overlay overlay -olowerdir=lower/,upperdir=upper/,workdir=work/,userxattr merged/
>> toybox getfattr -d merged/dir
>>
>> Then it will stuck on listxattr.
> I don't know what distro you're using, or what host filesystem types you're
> mounting together, and my devuan system hasn't got selinux installed:
>
>   root at driftwood:/home/landley# getenforce
>   bash: getenforce: command not found
>
> So when I create random files/directories on my laptop it doesn't attach xattrs
> to them by default, and thus I expect returning 0 is probably going to happen
> for me even on the overlay? (Or maybe -1. I'm not 100% certain my kernel or ext4
> filesystem driver are compiled with xattr support, although I _think_ they are?
> I tend to test this sort thing in mkroot where I can control all the variables,
> but that doesn't help me reproduce the issue the first time.)
>
> I'm just going to assume "Fedora" since that's the one with all the brittle
> infrastructure full of overcomplicated sharp edges. Things like selinux and
> systemd and glibc symbol versioning tend to be Red Hat's fault...

SELinux is not required for testing, the testcase will trigger an overlayfs
feature and attach upper/dir an xttr "*.overlay.origin".
I found this bug first on an android device: Android 13 + 5.15 kernel + F2FS
Then I tested it on a Ubuntu machine with Ext4 filesystem.

>>>> Also, I believe Elliott has a different implementation of getfattr in android
>>>> (for some sort of C++ library reasons, I'd have to check my notes), so he'll
>>>> probably want the test case there too?
>>> Would this approach work for you?
>> I compiled and tested your patch and it doesn't work, seems have some issues.
> I noticed this bug in the patch I sent you:
>
> $ ./getfattr doesnotexist
> getfattr: xrealloc
>
> Because I'm testing keys_len == -1 not len2.
>
> But I don't know what else is wrong with it yet, because I haven't got selinux
> here so there aren't any xattrs on my files. I'll try to reproduce it under a
> Fedora VM.

I add the len2 condition, and test case got an ERANGE.

>> As for malloc length problem, overlayfs filter out private attrs to hide it,
>> which means length decreasing is a possible situation but increasing not, so
> Good to know. Thanks. (I wonder what a private attr is? And why overlayfs
> DOESN'T filter them out when given a zero length, but does filter them out when
> given a buffer? Producing inconsistent results still sounds like an overlayfs
> bug in the kernel...)

OverlayFS has some particular xattrs to achieve its feature, so it's private
for overlayfs, it shows on upper files not merge/lower files. If overlayfs
get a zero buffer_size, it will return after vfs_listxattr and not filter
out private xattrs. If we want to change this behavior in the kernel, maybe
it will be tricky because of more memory allocation.

>> treat the increasing as an error might be a choice, what do you think of the
>> patch below:
> Your patch looks reasonable, but:
>
> $ diffstat mine
>  getfattr.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> $ diffstat yours
>  getfattr.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> The resulting code with your change is 11 lines longer than the resulting code
> with my change, plus the loop _does_ adjust for the file changing out from under
> us for other reasons, so I'd like to understand why the loop is failing.
>
> It sounds like overlayfs is providing different result lengths given different
> buffer lengths, and oscillates instead of stabilizing. Which does not fill me
> with confidence about the quality of this infratructure.
>
> Let me study your patch and try this on Fedora. (But first I'm downloading the
> Fedora 36 livecd to replace the 34 livecd I've been using, and it says it'll
> take 2 hours.)
>
> Rob

I check `man listxattr` examples and found it's similar to my approach:
first call to get current size and second call to update buffer_size.

IMO, we don't need a loop because twice listxattr is enough to fill the
buffer, what's your opinion?


Thanks,
Weizhao