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

[Rob Landley]

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...

>>> 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.

> 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...)

> 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