Re: [PATCH v2] hfsplus: don't store special "osx" xattr prefix on-disk

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Sun, 2015-04-05 at 00:36 -0400, Thomas Hebb wrote:
> This version of the commit is rebased on linux-next, as Andrew requested.
> The conflicting changes were just refactoring, so this version ought to
> be functionally identical to the first.
> 
> It does change the osx-namespace functions to call __hfsplus_?etxattr()
> directly instead of going through the refactored hfsplus_?etxattr()
> functions, which allocate space for a copy of the name with a prepended
> namespace. Since the osx attributes aren't stored with a namespace, we
> save ourselves a copy by bypassing the functions, but we also introduce
> the potential for future bugs if the hfsplus_?etattr() functions are
> later changed to do something that all namespaces need.
> 
> If the extra copy is inconsequential (or if there's a reason it's needed
> that I'm unaware of), I'm happy to revise the patch again to call
> hfsplus_?etattr() with an empty namespace parameter, instead of bypassing
> them altogether.

Looks good for me.

Thanks,
Vyacheslav Dubeyko.

> 
> Thanks,
> -Thomas
> 
> On Sat, 2015-04-04 at 19:08 -0400, Thomas Hebb wrote:
> > On Mac OS X, HFS+ extended attributes are not namespaced. Since we want
> > to be compatible with OS X filesystems and yet still support the Linux
> > namespacing system, the hfsplus driver implements a special "osx"
> > namespace that is reported for any attribute that is not namespaced
> > on-disk. However, the current code for getting and setting these
> > unprefixed attributes is broken.
> > 
> > hfsplus_osx_setattr() and hfsplus_osx_getattr() are passed names that
> > have already had their "osx." prefixes stripped by the generic functions.
> > The functions first, quite correctly, check those names to make sure
> > that they aren't prefixed with a known namespace, which would allow
> > namespace access restrictions to be bypassed. However, the functions
> > then prepend "osx." to the name they're given before passing it on to
> > hfsplus_getattr() and hfsplus_setattr(). Not only does this cause the
> > "osx." prefix to be stored on-disk, defeating its purpose, it also breaks
> > the check for the special "com.apple.FinderInfo" attribute, which is
> > reported for all files, and as a consequence makes some userspace
> > applications (e.g. GNU patch) fail even when extended attributes are not
> > otherwise in use.
> > 
> > There are five commits which have touched this particular code:
> > 
> >   127e5f5ae51e ("hfsplus: rework functionality of getting, setting and deleting of extended attributes")
> >   b168fff72109 ("hfsplus: use xattr handlers for removexattr")
> >   bf29e886b242 ("hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes")
> >   fcacbd95e121 ("fs/hfsplus: move xattr_name allocation in hfsplus_getxattr()")
> >   ec1bbd346f18 ("fs/hfsplus: move xattr_name allocation in hfsplus_setxattr()")
> > 
> > The first commit creates the functions to begin with. The namespace is
> > prepended by the original code, which I believe was correct at the time,
> > since hfsplus_?etattr() stripped the prefix if found. The second commit
> > removes this behavior from hfsplus_?etattr() and appears to have been
> > intended to also remove the prefixing from hfsplus_osx_?etattr(). However,
> > what it actually does is remove a necessary strncpy() call completely,
> > breaking the osx namespace entirely. The third commit re-adds the
> > strncpy() call as it was originally, but doesn't mention it in its
> > commit message. The final two commits refactor the code and don't
> > affect its functionality.
> > 
> > This commit does what b168fff attempted to do (prevent the prefix from
> > being added), but does it properly, instead of passing in an empty buffer
> > (which is what b168fff actually did).
> > 
> > Fixes: b168fff72109 ("hfsplus: use xattr handlers for removexattr")
> > Cc: stable@vger.kernel.org
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
> > Cc: Sergei Antonov <saproj@gmail.com>
> > Cc: Anton Altaparmakov <anton@tuxera.com>
> > Cc: Fabian Frederick <fabf@skynet.be>
> > Cc: Christian Kujau <lists@nerdbynature.de>
> > Signed-off-by: Thomas Hebb <tommyhebb@gmail.com>
> > ---
> >  fs/hfsplus/xattr.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> > index 8d62de0..89f262d 100644
> > --- a/fs/hfsplus/xattr.c
> > +++ b/fs/hfsplus/xattr.c
> > @@ -862,8 +862,13 @@ static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
> >         if (is_known_namespace(name))
> >                 return -EOPNOTSUPP;
> >  
> > -       return hfsplus_getxattr(dentry, name, buffer, size,
> > -                               XATTR_MAC_OSX_PREFIX, XATTR_MAC_OSX_PREFIX_LEN);
> > +       /*
> > +        * osx is the namespace we use to indicate an unprefixed
> > +        * attribute on the filesystem (like the ones that OS X
> > +        * creates), so we pass the name through unmodified (after
> > +        * ensuring it doesn't conflict with another namespace).
> > +        */
> > +       return __hfsplus_getxattr(dentry->d_inode, name, buffer, size);
> >  }
> >  
> >  static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
> > @@ -879,9 +884,13 @@ static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
> >         if (is_known_namespace(name))
> >                 return -EOPNOTSUPP;
> >  
> > -       return hfsplus_setxattr(dentry, name, buffer, size, flags,
> > -                               XATTR_MAC_OSX_PREFIX,
> > -                               XATTR_MAC_OSX_PREFIX_LEN);
> > +       /*
> > +        * osx is the namespace we use to indicate an unprefixed
> > +        * attribute on the filesystem (like the ones that OS X
> > +        * creates), so we pass the name through unmodified (after
> > +        * ensuring it doesn't conflict with another namespace).
> > +        */
> > +       return __hfsplus_setxattr(dentry->d_inode, name, buffer, size, flags);
> >  }
> >  
> >  static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,