linux-kernel - Re: [PATCH] Add basic export support to HFS+.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081209100619.GA18141@infradead.org>
Date:	Tue, 9 Dec 2008 05:06:19 -0500
From:	Christoph Hellwig <hch@...radead.org>
To:	Diego Elio 'Flameeyes' Petten?? <flameeyes@...il.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Add basic export support to HFS+.

On Thu, Dec 04, 2008 at 06:47:06PM +0100, Diego Elio 'Flameeyes' Petten?? wrote:
> From: Diego E. 'Flameeyes' Petten?? <flameeyes@...il.com>
> 
> The functions' skeleton is taken from ext2/super.c and seems to work
> fine for R/W access to HFS+ non-journaled case-sensitive filesystems.
> 
> It's probably slow and has a lot of space for improvement but it's
> still better than having no hope to export HFS+ filesystems.
> 
> Signed-off-by: Diego E. 'Flameeyes' Petten?? <flameeyes@...il.com>
> ---
> 
>  fs/hfsplus/super.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 

> +static struct inode *hfsplus_export_get_inode(struct super_block *sb,
> +					      u64 ino, u32 generation)
> +{
> +	struct inode *inode;
> +
> +	if (ino < HFSPLUS_FIRSTUSER_CNID && ino != HFSPLUS_ROOT_CNID)
> +		return ERR_PTR(-ESTALE);
> +
> +	/* iget isn't really right if the inode is currently unallocated!!
> +	 */

I don't understand this comment.  This interface is only used when
getting the FH for an already allocated inode.  hfsplus_iget is what
all other operations use in hfsplus, so it seems fine.  hfs experts
might chime in that something else might be needed due to the strange
ways links work in hfs, but from the VFS POW it seems fine.

> +	inode = hfsplus_iget(sb, ino);
> +	if (IS_ERR(inode))
> +		return ERR_CAST(inode);
> +	if (generation && inode->i_generation != generation) {
> +		/* we didn't find the right inode.. */
> +		iput(inode);
> +		return ERR_PTR(-ESTALE);
> +	}

hfsplus never updates i_generation, so this check is superflous.  Brad,
Roman: is there something that can help to guard against inode number
reuse on HFS/HFSplus?

> +/* Yes, most of these are left as NULL!!
> + * A NULL value implies the default, which (hopefully) works with
> + * hfs+-like file systems, but can be improved upon.
> + * Currently only fh_to_dentry is required.
> + */

You _do_ need a get_parent, otherwise access will magically fail once
any directory inode in a path is out of the cache.  This can be
trivially reproduced by unexporting and unmounting a filesystem while
a client has a cwd deep inside the exported filesystem.

And otherwise I don't think this comment is helpful in the code,
fh_to_parent / fh_To_dentry and get_parent is what most simpler
filesystems have.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/