| Message ID | 1428188908.13461.0.camel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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, -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)