[LU-12371] Odd behavior on lustre client listxattr for directories

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.7.0, Lustre 2.10.8
• Type: Bug
• Priority: Minor
• Reporter: Kristin Webb
• Assignee: Andreas Dilger
• Resolution: Won't Fix
• Labels: easy

Environment

CentOS 6 & 7, Vmware workstation virtual machine test environement

• Epic/Theme: client
• Epic: client

Description

Our backup software found an unusual result when listing xattrs for directories.

On the client side, when requesting the buffer size for the list:

listxattr("foo", NULL, 0)               = 36 (bytes)

But when the list is actually read, it comes up short:

listxattr("foo", "trusted.link\0trusted.lma\0", 256) = 25 (bytes)

On the server side, on an ldiskfs mount, the results are more consistent.

listxattr("foo", NULL, 0)               = 25
listxattr("foo", "trusted.lma\0trusted.link\0", 256) = 25

It seems like a bug in the following piece of code:

ssize_t ll_listxattr(struct dentry *dentry, char *buffer, size_t size)
{

        /*
         * If we're being called to get the size of the xattr list
         * (size == 0) then just assume that a lustre.lov xattr
         * exists.
         */
        if (!size)
                RETURN(rc + sizeof(XATTR_LUSTRE_LOV));

This is true for regular files, but not directories (or at least not all
of them).  As a starting point, something like the following should fix
the immediate problem you see:

        if (!size) {
                rc += S_ISREG(inode->i_mode) ? sizeof(XATTR_LUSTRE_LOV) : 0;
                RETURN(rc);
        }

though it is still not wholly correct.  We should go through xattr_type_filter()
in all cases, so that e.g. it filters out trusted.* xattrs for regular users.
I guess it depends on whether one considers the returned buffer size "large
enough to hold all xattrs", or "exactly the size of the returned xattrs".  The
latter is definitely more forgiving and could be fixed immediately in your code,
rather than depending on a fix to propagate into all of the installed Lustre
systems you are running on.

Comments

[Andreas Dilger]

Kristin,
I looked into this code a bit further, and it isn't clear that it is practical to avoid inflating the xattr list size in this manner without adding overhead to this operation. As it stands, returning the too-large xattr list size is trivial to execute, and is safe because at worst the buffer is allocated a few bytes larger than needed. The proposed patch in my original comment is mostly correct, but does not handle the case where a directory has a default file layout attached, so would return a too-small buffer size, and likely cause the caller to be unhappy and/or corrupt memory when the list is too long (== as implemented today).

Also, while the likelihood is rare, it is possible for there to be a race condition between calling listxattr(list=NULL) to fetch the buffer size, and listxattr() with the allocated buffer to fetch the actual xattr names. This means that the list size may be larger or smaller than listxattr() originally reported, so it is safer to assume that the returned buffer size is a reasonable estimate, but have retry logic in the caller if an error is returned like ERANGE indicating the buffer is too small, and be accepting if the buffer is too large.

[Kristin Webb]

I agree.  I also see the race condition between calls and that is something that software that manages/uses xattr's needs to be aware of.  Thank you for taking a look into this.  Do I need to do anything to close out the ticket?

Kris

[Gerrit Updater]

Andreas Dilger ([e-mail]) uploaded a new patch: https://review.whamcloud.com/39791
Subject: LU-12371 llite: don't inflate listxattr for dirs
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 0b25d5d00c8e1b5131b78231a16082f61bdc1379