[PATCH] hfsplus: Remove hfsplus_file_lookup

[PATCH] hfsplus: Remove hfsplus_file_lookup

[Sougata Santra]

From: Sougata Santra <sougata@tuxera.com>

HFS+ resource fork lookup breaks opendir() library function. Since
opendir first calls open() with O_DIRECTORY flag set. O_DIRECTORY
means "refuse to open if not a directory". The open system call
in the kernel does a check for inode->i_op->lookup and returns
-ENOTDIR. So if hfsplus_file_lookup is set it allows opendir()
for plain files.

Also resource fork lookup in HFS+ does not work. Since it is never
invoked after VFS permission checking. It will always return with
-EACCES.

Acked-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sougata Santra <sougata@tuxera.com>
---

When we call opendir() on a file, it does not return NULL.
opendir() library call is based on open with O_DIRECTORY flag
passed and then layered on top of getdents() system call.
O_DIRECTORY means "refuse to open if not a directory".

The open() system call in the kernel does a check for:
do_sys_open() -->..--> can_lookup() i.e it only checks
inode->i_op->lookup and returns ENOTDIR if
this function pointer is not set.
"
In OSX, we can open "file/rsrc" to get the resource fork of "file".
This behavior is emulated inside hfsplus on Linux, which means that
to some degree every file acts like a directory. That is the reason
lookup() inode operations is supported for files, and it is possible
to do a lookup on this specific name. As a result of this open succeeds
without returning  ENOTDIR for HFS+
"
Please see the LKML discussion thread on this issue:
http://marc.info/?l=linux-fsdevel&m=122823343730412&w=2 

I tried to test file/rsrc lookup in HFS+ driver and the feature does not work.
>From OSX:
$ touch test
$ echo "1234" > test/..namedfork/rsrc
$ ls -l test..namedfork/rsrc
--rw-r--r-- 1 tuxera staff 5 10 dec 12:59 test/..namedfork/rsrc

[sougata@ultrabook tmp]$ id
uid=1000(sougata) gid=1000(sougata) groups=1000(sougata),5(tty),18(dialout),1001(vboxusers)

[sougata@ultrabook tmp]$ mount
/dev/sdb1 on /mnt/tmp type hfsplus (rw,relatime,umask=0,uid=1000,gid=1000,nls=utf8)

[sougata@ultrabook tmp]$ ls -l test/rsrc
ls: cannot access test/rsrc: Permission denied

According to this LKML thread it is expected behavior.
http://marc.info/?t=121139033800008&r=1&w=4
I guess now that permission checking happens in vfs generic_permission() ?
So it turns out that even though the lookup() inode_operation exists for HFS+ files.
It cannot really get invoked ?. So if we can disable this feature to make opendir()
work for HFS+.

Thanks,
    Sougata

 fs/hfsplus/inode.c | 59 ------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 37213d0..3ebda92 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -178,64 +178,6 @@ const struct dentry_operations hfsplus_dentry_operations = {
 	.d_compare    = hfsplus_compare_dentry,
 };
 
-static struct dentry *hfsplus_file_lookup(struct inode *dir,
-		struct dentry *dentry, unsigned int flags)
-{
-	struct hfs_find_data fd;
-	struct super_block *sb = dir->i_sb;
-	struct inode *inode = NULL;
-	struct hfsplus_inode_info *hip;
-	int err;
-
-	if (HFSPLUS_IS_RSRC(dir) || strcmp(dentry->d_name.name, "rsrc"))
-		goto out;
-
-	inode = HFSPLUS_I(dir)->rsrc_inode;
-	if (inode)
-		goto out;
-
-	inode = new_inode(sb);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	hip = HFSPLUS_I(inode);
-	inode->i_ino = dir->i_ino;
-	INIT_LIST_HEAD(&hip->open_dir_list);
-	mutex_init(&hip->extents_lock);
-	hip->extent_state = 0;
-	hip->flags = 0;
-	hip->userflags = 0;
-	set_bit(HFSPLUS_I_RSRC, &hip->flags);
-
-	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
-	if (!err) {
-		err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-		if (!err)
-			err = hfsplus_cat_read_inode(inode, &fd);
-		hfs_find_exit(&fd);
-	}
-	if (err) {
-		iput(inode);
-		return ERR_PTR(err);
-	}
-	hip->rsrc_inode = dir;
-	HFSPLUS_I(dir)->rsrc_inode = inode;
-	igrab(dir);
-
-	/*
-	 * __mark_inode_dirty expects inodes to be hashed.  Since we don't
-	 * want resource fork inodes in the regular inode space, we make them
-	 * appear hashed, but do not put on any lists.  hlist_del()
-	 * will work fine and require no locking.
-	 */
-	hlist_add_fake(&inode->i_hash);
-
-	mark_inode_dirty(inode);
-out:
-	d_add(dentry, inode);
-	return NULL;
-}
-
 static void hfsplus_get_perms(struct inode *inode,
 		struct hfsplus_perm *perms, int dir)
 {
@@ -385,7 +327,6 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 }
 
 static const struct inode_operations hfsplus_file_inode_operations = {
-	.lookup		= hfsplus_file_lookup,
 	.setattr	= hfsplus_setattr,
 	.setxattr	= generic_setxattr,
 	.getxattr	= generic_getxattr,
-- 
1.8.1.4

[PATCH] hfsplus: Remove hfsplus_file_lookup

[Sougata Santra]

From: Sougata Santra <sougata@tuxera.com>

HFS+ resource fork lookup breaks opendir() library function. Since
opendir first calls open() with O_DIRECTORY flag set. O_DIRECTORY
means "refuse to open if not a directory". The open system call
in the kernel does a check for inode->i_op->lookup and returns
-ENOTDIR. So if hfsplus_file_lookup is set it allows opendir()
for plain files.

Also resource fork lookup in HFS+ does not work. Since it is never
invoked after VFS permission checking. It will always return with
-EACCES.

Acked-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sougata Santra <sougata@tuxera.com>
---

When we call opendir() on a file, it does not return NULL.
opendir() library call is based on open with O_DIRECTORY flag
passed and then layered on top of getdents() system call.
O_DIRECTORY means "refuse to open if not a directory".

The open() system call in the kernel does a check for:
do_sys_open() -->..--> can_lookup() i.e it only checks
inode->i_op->lookup and returns ENOTDIR if
this function pointer is not set.
"
In OSX, we can open "file/rsrc" to get the resource fork of "file".
This behavior is emulated inside hfsplus on Linux, which means that
to some degree every file acts like a directory. That is the reason
lookup() inode operations is supported for files, and it is possible
to do a lookup on this specific name. As a result of this open succeeds
without returning  ENOTDIR for HFS+
"
Please see the LKML discussion thread on this issue:
http://marc.info/?l=linux-fsdevel&m=122823343730412&w=2 

I tried to test file/rsrc lookup in HFS+ driver and the feature does not work.
>From OSX:
$ touch test
$ echo "1234" > test/..namedfork/rsrc
$ ls -l test..namedfork/rsrc
--rw-r--r-- 1 tuxera staff 5 10 dec 12:59 test/..namedfork/rsrc

[sougata@ultrabook tmp]$ id
uid=1000(sougata) gid=1000(sougata) groups=1000(sougata),5(tty),18(dialout),1001(vboxusers)

[sougata@ultrabook tmp]$ mount
/dev/sdb1 on /mnt/tmp type hfsplus (rw,relatime,umask=0,uid=1000,gid=1000,nls=utf8)

[sougata@ultrabook tmp]$ ls -l test/rsrc
ls: cannot access test/rsrc: Permission denied

According to this LKML thread it is expected behavior.
http://marc.info/?t=121139033800008&r=1&w=4
I guess now that permission checking happens in vfs generic_permission() ?
So it turns out that even though the lookup() inode_operation exists for HFS+ files.
It cannot really get invoked ?. So if we can disable this feature to make opendir()
work for HFS+.

Thanks,
    Sougata

 fs/hfsplus/inode.c | 59 ------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 37213d0..3ebda92 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -178,64 +178,6 @@ const struct dentry_operations hfsplus_dentry_operations = {
 	.d_compare    = hfsplus_compare_dentry,
 };
 
-static struct dentry *hfsplus_file_lookup(struct inode *dir,
-		struct dentry *dentry, unsigned int flags)
-{
-	struct hfs_find_data fd;
-	struct super_block *sb = dir->i_sb;
-	struct inode *inode = NULL;
-	struct hfsplus_inode_info *hip;
-	int err;
-
-	if (HFSPLUS_IS_RSRC(dir) || strcmp(dentry->d_name.name, "rsrc"))
-		goto out;
-
-	inode = HFSPLUS_I(dir)->rsrc_inode;
-	if (inode)
-		goto out;
-
-	inode = new_inode(sb);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	hip = HFSPLUS_I(inode);
-	inode->i_ino = dir->i_ino;
-	INIT_LIST_HEAD(&hip->open_dir_list);
-	mutex_init(&hip->extents_lock);
-	hip->extent_state = 0;
-	hip->flags = 0;
-	hip->userflags = 0;
-	set_bit(HFSPLUS_I_RSRC, &hip->flags);
-
-	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
-	if (!err) {
-		err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-		if (!err)
-			err = hfsplus_cat_read_inode(inode, &fd);
-		hfs_find_exit(&fd);
-	}
-	if (err) {
-		iput(inode);
-		return ERR_PTR(err);
-	}
-	hip->rsrc_inode = dir;
-	HFSPLUS_I(dir)->rsrc_inode = inode;
-	igrab(dir);
-
-	/*
-	 * __mark_inode_dirty expects inodes to be hashed.  Since we don't
-	 * want resource fork inodes in the regular inode space, we make them
-	 * appear hashed, but do not put on any lists.  hlist_del()
-	 * will work fine and require no locking.
-	 */
-	hlist_add_fake(&inode->i_hash);
-
-	mark_inode_dirty(inode);
-out:
-	d_add(dentry, inode);
-	return NULL;
-}
-
 static void hfsplus_get_perms(struct inode *inode,
 		struct hfsplus_perm *perms, int dir)
 {
@@ -385,7 +327,6 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 }
 
 static const struct inode_operations hfsplus_file_inode_operations = {
-	.lookup		= hfsplus_file_lookup,
 	.setattr	= hfsplus_setattr,
 	.setxattr	= generic_setxattr,
 	.getxattr	= generic_getxattr,
-- 
1.8.1.4

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 10:35:01AM +0400, Vyacheslav Dubeyko wrote:
> I think that I can implement support of resource forks by means of xattr
> way. Also, currently, I am implementing HFS+ compressed files support.
> So, I can clean up old-fashioned way of resource forks support in HFS+
> driver because of necessity to rework it anyway. The suggested patch
> doesn't make all necessary cleanup, from my viewpoint.
> 
> Any comments?

The patch is required band aid to fix easily user visible and
triggerable breakage.  As such it should go in ASAP and not be blocked
by additional cleanups.  That doesn't mean that I wouldn't love to see
your planned rework and additional cleanups later.

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 10:35:01AM +0400, Vyacheslav Dubeyko wrote:
> I think that I can implement support of resource forks by means of xattr
> way. Also, currently, I am implementing HFS+ compressed files support.
> So, I can clean up old-fashioned way of resource forks support in HFS+
> driver because of necessity to rework it anyway. The suggested patch
> doesn't make all necessary cleanup, from my viewpoint.
> 
> Any comments?

The patch is required band aid to fix easily user visible and
triggerable breakage.  As such it should go in ASAP and not be blocked
by additional cleanups.  That doesn't mean that I wouldn't love to see
your planned rework and additional cleanups later.

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Christoph Hellwig]

The opendir issue is something that came up before, both in the reiser4
context and with hfsplus.  I think we'll need to put this patch in ASAP
to fix the semantic breakage caused by it, as well as other implications
of having ->lookup on a hardlinkable object.


Acked-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Vyacheslav Dubeyko]

On Wed, 2013-12-11 at 21:08 +0000, Anton Altaparmakov wrote:
> Hi,
> 
> On 11 Dec 2013, at 19:11, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:
> >> This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
> >> or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
> >> report.
> > 
> > This "feature" is severely broken and yes, outright removal is what I'd
> > suggest for a fix.  HFS+ allows hardlinks to files, which means that
> > you allow multiple dentries for the same inode with ->lookup() in it,
> > which is asking for deadlocks.
> > 
> > This is fundamentally not supported.  Considering that forks are lousy
> > idea in the first place, I'd seriously suggest to remove that idiocy for
> > good.
> 
> Completely agree with Al.  If anyone really wants access to forks they can implement them via the xattr interface (ok it has the 64k limitation but most forks are quite small so not much of an issue).  That's how I implemented access to named streams in Tuxera NTFS and it works a treat (and allows Linux apps and various security modules that require xattr support to work properly which is also great).
> 

Yes, I have the same considerations about using xattr way for the case
of resource fork after the night.

Usually, a file under HFS+ has or valid data fork, or valid resource
fork. So, HFS+ compressed file has valid resource fork only. Also alias
under Mac OS X has valid resource fork only. Of course, regular file can
have as valid data fork as valid resource fork. Fortunately, such case
is rare now (when file has both forks are valid). So, we can use xattr
way for accessing resource fork for such files. For example, it is
possible to use "osx.ResourceFork" xattr's name. And I suppose that 64
KB is reasonable limitation. Now we have access to FinderInfo fields of
CatalogFile's record for file under HFS+ by means of
"com.apple.FinderInfo" xattr.

I think that I can implement support of resource forks by means of xattr
way. Also, currently, I am implementing HFS+ compressed files support.
So, I can clean up old-fashioned way of resource forks support in HFS+
driver because of necessity to rework it anyway. The suggested patch
doesn't make all necessary cleanup, from my viewpoint.

Any comments?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Vyacheslav Dubeyko]

On Wed, 2013-12-11 at 21:08 +0000, Anton Altaparmakov wrote:
> Hi,
> 
> On 11 Dec 2013, at 19:11, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:
> >> This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
> >> or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
> >> report.
> > 
> > This "feature" is severely broken and yes, outright removal is what I'd
> > suggest for a fix.  HFS+ allows hardlinks to files, which means that
> > you allow multiple dentries for the same inode with ->lookup() in it,
> > which is asking for deadlocks.
> > 
> > This is fundamentally not supported.  Considering that forks are lousy
> > idea in the first place, I'd seriously suggest to remove that idiocy for
> > good.
> 
> Completely agree with Al.  If anyone really wants access to forks they can implement them via the xattr interface (ok it has the 64k limitation but most forks are quite small so not much of an issue).  That's how I implemented access to named streams in Tuxera NTFS and it works a treat (and allows Linux apps and various security modules that require xattr support to work properly which is also great).
> 

Yes, I have the same considerations about using xattr way for the case
of resource fork after the night.

Usually, a file under HFS+ has or valid data fork, or valid resource
fork. So, HFS+ compressed file has valid resource fork only. Also alias
under Mac OS X has valid resource fork only. Of course, regular file can
have as valid data fork as valid resource fork. Fortunately, such case
is rare now (when file has both forks are valid). So, we can use xattr
way for accessing resource fork for such files. For example, it is
possible to use "osx.ResourceFork" xattr's name. And I suppose that 64
KB is reasonable limitation. Now we have access to FinderInfo fields of
CatalogFile's record for file under HFS+ by means of
"com.apple.FinderInfo" xattr.

I think that I can implement support of resource forks by means of xattr
way. Also, currently, I am implementing HFS+ compressed files support.
So, I can clean up old-fashioned way of resource forks support in HFS+
driver because of necessity to rework it anyway. The suggested patch
doesn't make all necessary cleanup, from my viewpoint.

Any comments?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Anton Altaparmakov]

Hi,

On 11 Dec 2013, at 19:11, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:
>> This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
>> or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
>> report.
> 
> This "feature" is severely broken and yes, outright removal is what I'd
> suggest for a fix.  HFS+ allows hardlinks to files, which means that
> you allow multiple dentries for the same inode with ->lookup() in it,
> which is asking for deadlocks.
> 
> This is fundamentally not supported.  Considering that forks are lousy
> idea in the first place, I'd seriously suggest to remove that idiocy for
> good.

Completely agree with Al.  If anyone really wants access to forks they can implement them via the xattr interface (ok it has the 64k limitation but most forks are quite small so not much of an issue).  That's how I implemented access to named streams in Tuxera NTFS and it works a treat (and allows Linux apps and various security modules that require xattr support to work properly which is also great).

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Anton Altaparmakov]

Hi,

On 11 Dec 2013, at 19:11, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:
>> This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
>> or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
>> report.
> 
> This "feature" is severely broken and yes, outright removal is what I'd
> suggest for a fix.  HFS+ allows hardlinks to files, which means that
> you allow multiple dentries for the same inode with ->lookup() in it,
> which is asking for deadlocks.
> 
> This is fundamentally not supported.  Considering that forks are lousy
> idea in the first place, I'd seriously suggest to remove that idiocy for
> good.

Completely agree with Al.  If anyone really wants access to forks they can implement them via the xattr interface (ok it has the 64k limitation but most forks are quite small so not much of an issue).  That's how I implemented access to named streams in Tuxera NTFS and it works a treat (and allows Linux apps and various security modules that require xattr support to work properly which is also great).

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Vyacheslav Dubeyko]

On Dec 11, 2013, at 7:56 PM, Sougata Santra wrote:

[snip]
> In OSX, we can open "file/rsrc" to get the resource fork of "file".
> This behavior is emulated inside hfsplus on Linux, which means that
> to some degree every file acts like a directory. That is the reason
> lookup() inode operations is supported for files, and it is possible
> to do a lookup on this specific name. As a result of this open succeeds
> without returning  ENOTDIR for HFS+
> "
> Please see the LKML discussion thread on this issue:
> http://marc.info/?l=linux-fsdevel&m=122823343730412&w=2 
> 
> I tried to test file/rsrc lookup in HFS+ driver and the feature does not work.
> From OSX:
> $ touch test
> $ echo "1234" > test/..namedfork/rsrc
> $ ls -l test..namedfork/rsrc
> --rw-r--r-- 1 tuxera staff 5 10 dec 12:59 test/..namedfork/rsrc
> 
> [sougata@ultrabook tmp]$ id
> uid=1000(sougata) gid=1000(sougata) groups=1000(sougata),5(tty),18(dialout),1001(vboxusers)
> 
> [sougata@ultrabook tmp]$ mount
> /dev/sdb1 on /mnt/tmp type hfsplus (rw,relatime,umask=0,uid=1000,gid=1000,nls=utf8)
> 
> [sougata@ultrabook tmp]$ ls -l test/rsrc
> ls: cannot access test/rsrc: Permission denied
> 
> According to this LKML thread it is expected behavior.
> http://marc.info/?t=121139033800008&r=1&w=4
> I guess now that permission checking happens in vfs generic_permission() ?
> So it turns out that even though the lookup() inode_operation exists for HFS+ files.
> It cannot really get invoked ?. So if we can disable this feature to make opendir()
> work for HFS+.
> 

This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
report.

But I think that this patch can't be the solution of the issue. So, if you want then
you can investigate and fix the issue. Could you do it?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Vyacheslav Dubeyko]

On Dec 11, 2013, at 7:56 PM, Sougata Santra wrote:

[snip]
> In OSX, we can open "file/rsrc" to get the resource fork of "file".
> This behavior is emulated inside hfsplus on Linux, which means that
> to some degree every file acts like a directory. That is the reason
> lookup() inode operations is supported for files, and it is possible
> to do a lookup on this specific name. As a result of this open succeeds
> without returning  ENOTDIR for HFS+
> "
> Please see the LKML discussion thread on this issue:
> http://marc.info/?l=linux-fsdevel&m=122823343730412&w=2 
> 
> I tried to test file/rsrc lookup in HFS+ driver and the feature does not work.
> From OSX:
> $ touch test
> $ echo "1234" > test/..namedfork/rsrc
> $ ls -l test..namedfork/rsrc
> --rw-r--r-- 1 tuxera staff 5 10 dec 12:59 test/..namedfork/rsrc
> 
> [sougata@ultrabook tmp]$ id
> uid=1000(sougata) gid=1000(sougata) groups=1000(sougata),5(tty),18(dialout),1001(vboxusers)
> 
> [sougata@ultrabook tmp]$ mount
> /dev/sdb1 on /mnt/tmp type hfsplus (rw,relatime,umask=0,uid=1000,gid=1000,nls=utf8)
> 
> [sougata@ultrabook tmp]$ ls -l test/rsrc
> ls: cannot access test/rsrc: Permission denied
> 
> According to this LKML thread it is expected behavior.
> http://marc.info/?t=121139033800008&r=1&w=4
> I guess now that permission checking happens in vfs generic_permission() ?
> So it turns out that even though the lookup() inode_operation exists for HFS+ files.
> It cannot really get invoked ?. So if we can disable this feature to make opendir()
> work for HFS+.
> 

This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
report.

But I think that this patch can't be the solution of the issue. So, if you want then
you can investigate and fix the issue. Could you do it?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: Remove hfsplus_file_lookup

[Al Viro]

On Wed, Dec 11, 2013 at 10:49:29PM +0300, Vyacheslav Dubeyko wrote:

> This feature worked earlier under Linux. So, I suppose that some changes in HFS+ driver
> or in VFS broke it. And it needs to investigate and fix the reported issue. Thank you for the
> report.

This "feature" is severely broken and yes, outright removal is what I'd
suggest for a fix.  HFS+ allows hardlinks to files, which means that
you allow multiple dentries for the same inode with ->lookup() in it,
which is asking for deadlocks.

This is fundamentally not supported.  Considering that forks are lousy
idea in the first place, I'd seriously suggest to remove that idiocy for
good.

[PATCH] hfsplus: Remove hfsplus_file_lookup

[Sougata Santra]

From: Sougata Santra <sougata@tuxera.com>

HFS+ resource fork lookup breaks opendir() library function. Since
opendir first calls open() with O_DIRECTORY flag set. O_DIRECTORY
means "refuse to open if not a directory". The open system call
in the kernel does a check for inode->i_op->lookup and returns
-ENOTDIR. So if hfsplus_file_lookup is set it allows opendir()
for plain files.

Also resource fork lookup in HFS+ does not work. Since it is never
invoked after VFS permission checking. It will always return with
-EACCES.

Signed-off-by: Sougata Santra <sougata@tuxera.com>
---

When we call opendir() on a file, it does not return NULL.
opendir() library call is based on open with O_DIRECTORY flag
passed and then layered on top of getdents() system call.
O_DIRECTORY means "refuse to open if not a directory".

The open() system call in the kernel does a check for:
do_sys_open() -->..--> can_lookup() i.e it only checks
inode->i_op->lookup and returns ENOTDIR if
this function pointer is not set.
"
In OSX, we can open "file/rsrc" to get the resource fork of "file".
This behavior is emulated inside hfsplus on Linux, which means that
to some degree every file acts like a directory. That is the reason
lookup() inode operations is supported for files, and it is possible
to do a lookup on this specific name. As a result of this open succeeds
without returning  ENOTDIR for HFS+
"
Please see the LKML discussion thread on this issue:
http://marc.info/?l=linux-fsdevel&m=122823343730412&w=2 

I tried to test file/rsrc lookup in HFS+ driver and the feature does not work.
>From OSX:
$ touch test
$ echo "1234" > test/..namedfork/rsrc
$ ls -l test..namedfork/rsrc
--rw-r--r-- 1 tuxera staff 5 10 dec 12:59 test/..namedfork/rsrc

[sougata@ultrabook tmp]$ id
uid=1000(sougata) gid=1000(sougata) groups=1000(sougata),5(tty),18(dialout),1001(vboxusers)

[sougata@ultrabook tmp]$ mount
/dev/sdb1 on /mnt/tmp type hfsplus (rw,relatime,umask=0,uid=1000,gid=1000,nls=utf8)

[sougata@ultrabook tmp]$ ls -l test/rsrc
ls: cannot access test/rsrc: Permission denied

According to this LKML thread it is expected behavior.
http://marc.info/?t=121139033800008&r=1&w=4
I guess now that permission checking happens in vfs generic_permission() ?
So it turns out that even though the lookup() inode_operation exists for HFS+ files.
It cannot really get invoked ?. So if we can disable this feature to make opendir()
work for HFS+.

Thanks,
    Sougata

 fs/hfsplus/inode.c | 59 ------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 37213d0..3ebda92 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -178,64 +178,6 @@ const struct dentry_operations hfsplus_dentry_operations = {
 	.d_compare    = hfsplus_compare_dentry,
 };
 
-static struct dentry *hfsplus_file_lookup(struct inode *dir,
-		struct dentry *dentry, unsigned int flags)
-{
-	struct hfs_find_data fd;
-	struct super_block *sb = dir->i_sb;
-	struct inode *inode = NULL;
-	struct hfsplus_inode_info *hip;
-	int err;
-
-	if (HFSPLUS_IS_RSRC(dir) || strcmp(dentry->d_name.name, "rsrc"))
-		goto out;
-
-	inode = HFSPLUS_I(dir)->rsrc_inode;
-	if (inode)
-		goto out;
-
-	inode = new_inode(sb);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	hip = HFSPLUS_I(inode);
-	inode->i_ino = dir->i_ino;
-	INIT_LIST_HEAD(&hip->open_dir_list);
-	mutex_init(&hip->extents_lock);
-	hip->extent_state = 0;
-	hip->flags = 0;
-	hip->userflags = 0;
-	set_bit(HFSPLUS_I_RSRC, &hip->flags);
-
-	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
-	if (!err) {
-		err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-		if (!err)
-			err = hfsplus_cat_read_inode(inode, &fd);
-		hfs_find_exit(&fd);
-	}
-	if (err) {
-		iput(inode);
-		return ERR_PTR(err);
-	}
-	hip->rsrc_inode = dir;
-	HFSPLUS_I(dir)->rsrc_inode = inode;
-	igrab(dir);
-
-	/*
-	 * __mark_inode_dirty expects inodes to be hashed.  Since we don't
-	 * want resource fork inodes in the regular inode space, we make them
-	 * appear hashed, but do not put on any lists.  hlist_del()
-	 * will work fine and require no locking.
-	 */
-	hlist_add_fake(&inode->i_hash);
-
-	mark_inode_dirty(inode);
-out:
-	d_add(dentry, inode);
-	return NULL;
-}
-
 static void hfsplus_get_perms(struct inode *inode,
 		struct hfsplus_perm *perms, int dir)
 {
@@ -385,7 +327,6 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 }
 
 static const struct inode_operations hfsplus_file_inode_operations = {
-	.lookup		= hfsplus_file_lookup,
 	.setattr	= hfsplus_setattr,
 	.setxattr	= generic_setxattr,
 	.getxattr	= generic_getxattr,
-- 
1.8.1.4

[PATCH] hfsplus: Remove hfsplus_file_lookup

[Sougata Santra]

From: Sougata Santra <sougata@tuxera.com>

HFS+ resource fork lookup breaks opendir() library function. Since
opendir first calls open() with O_DIRECTORY flag set. O_DIRECTORY
means "refuse to open if not a directory". The open system call
in the kernel does a check for inode->i_op->lookup and returns
-ENOTDIR. So if hfsplus_file_lookup is set it allows opendir()
for plain files.

Also resource fork lookup in HFS+ does not work. Since it is never
invoked after VFS permission checking. It will always return with
-EACCES.

Signed-off-by: Sougata Santra <sougata@tuxera.com>
---

When we call opendir() on a file, it does not return NULL.
opendir() library call is based on open with O_DIRECTORY flag
passed and then layered on top of getdents() system call.
O_DIRECTORY means "refuse to open if not a directory".

The open() system call in the kernel does a check for:
do_sys_open() -->..--> can_lookup() i.e it only checks
inode->i_op->lookup and returns ENOTDIR if
this function pointer is not set.
"
In OSX, we can open "file/rsrc" to get the resource fork of "file".
This behavior is emulated inside hfsplus on Linux, which means that
to some degree every file acts like a directory. That is the reason
lookup() inode operations is supported for files, and it is possible
to do a lookup on this specific name. As a result of this open succeeds
without returning  ENOTDIR for HFS+
"
Please see the LKML discussion thread on this issue:
http://marc.info/?l=linux-fsdevel&m=122823343730412&w=2 

I tried to test file/rsrc lookup in HFS+ driver and the feature does not work.
>From OSX:
$ touch test
$ echo "1234" > test/..namedfork/rsrc
$ ls -l test..namedfork/rsrc
--rw-r--r-- 1 tuxera staff 5 10 dec 12:59 test/..namedfork/rsrc

[sougata@ultrabook tmp]$ id
uid=1000(sougata) gid=1000(sougata) groups=1000(sougata),5(tty),18(dialout),1001(vboxusers)

[sougata@ultrabook tmp]$ mount
/dev/sdb1 on /mnt/tmp type hfsplus (rw,relatime,umask=0,uid=1000,gid=1000,nls=utf8)

[sougata@ultrabook tmp]$ ls -l test/rsrc
ls: cannot access test/rsrc: Permission denied

According to this LKML thread it is expected behavior.
http://marc.info/?t=121139033800008&r=1&w=4
I guess now that permission checking happens in vfs generic_permission() ?
So it turns out that even though the lookup() inode_operation exists for HFS+ files.
It cannot really get invoked ?. So if we can disable this feature to make opendir()
work for HFS+.

Thanks,
    Sougata

 fs/hfsplus/inode.c | 59 ------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 37213d0..3ebda92 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -178,64 +178,6 @@ const struct dentry_operations hfsplus_dentry_operations = {
 	.d_compare    = hfsplus_compare_dentry,
 };
 
-static struct dentry *hfsplus_file_lookup(struct inode *dir,
-		struct dentry *dentry, unsigned int flags)
-{
-	struct hfs_find_data fd;
-	struct super_block *sb = dir->i_sb;
-	struct inode *inode = NULL;
-	struct hfsplus_inode_info *hip;
-	int err;
-
-	if (HFSPLUS_IS_RSRC(dir) || strcmp(dentry->d_name.name, "rsrc"))
-		goto out;
-
-	inode = HFSPLUS_I(dir)->rsrc_inode;
-	if (inode)
-		goto out;
-
-	inode = new_inode(sb);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	hip = HFSPLUS_I(inode);
-	inode->i_ino = dir->i_ino;
-	INIT_LIST_HEAD(&hip->open_dir_list);
-	mutex_init(&hip->extents_lock);
-	hip->extent_state = 0;
-	hip->flags = 0;
-	hip->userflags = 0;
-	set_bit(HFSPLUS_I_RSRC, &hip->flags);
-
-	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
-	if (!err) {
-		err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-		if (!err)
-			err = hfsplus_cat_read_inode(inode, &fd);
-		hfs_find_exit(&fd);
-	}
-	if (err) {
-		iput(inode);
-		return ERR_PTR(err);
-	}
-	hip->rsrc_inode = dir;
-	HFSPLUS_I(dir)->rsrc_inode = inode;
-	igrab(dir);
-
-	/*
-	 * __mark_inode_dirty expects inodes to be hashed.  Since we don't
-	 * want resource fork inodes in the regular inode space, we make them
-	 * appear hashed, but do not put on any lists.  hlist_del()
-	 * will work fine and require no locking.
-	 */
-	hlist_add_fake(&inode->i_hash);
-
-	mark_inode_dirty(inode);
-out:
-	d_add(dentry, inode);
-	return NULL;
-}
-
 static void hfsplus_get_perms(struct inode *inode,
 		struct hfsplus_perm *perms, int dir)
 {
@@ -385,7 +327,6 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 }
 
 static const struct inode_operations hfsplus_file_inode_operations = {
-	.lookup		= hfsplus_file_lookup,
 	.setattr	= hfsplus_setattr,
 	.setxattr	= generic_setxattr,
 	.getxattr	= generic_getxattr,
-- 
1.8.1.4