linux-kernel - Re: [PATCH 2/2] fat (exportfs): reconnect file handles to evicted inodes/dentries

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: <87pq8bokcp.fsf@devron.myhome.or.jp>
Date:	Wed, 04 Jul 2012 20:07:34 +0900
From:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:	"Steven J. Magnani" <steve@...idescorp.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] fat (exportfs): reconnect file handles to evicted inodes/dentries

"Steven J. Magnani" <steve@...idescorp.com> writes:

> This patch adds code to support reconstruction of evicted inodes.
> We walk the on-disk structures where necessary to fill in information
> not available in the NFS file handle.
>
> One important point is that when reconstructing an inode, in order to avoid the
> *client* declaring ESTALE we have to ensure that the NFS file handle of the
> reconstruction is identical to that of the original. 

Sorry, I didn't review fully yet though.

We would like to add the comment of [0/2] explanation here. Here is
missing the explanation of the problem.

And the codes of nfs support became larger, and I think we should give
the place for it. I.e. we would like to add new file (maybe, export.c,
nfs.c, or something), move nfs code into it.

With it, we don't need to add "NFS ..." annotation in the comment, and
it would make more clear.

And can you add explanation the test of this? What were tested?

> +/**
> + * NFS helper: retrieve the name of an anonymous (disconnected) child using
> + * its i_pos or i_logstart and knowledge of its parent
> + *
> + * Returns 0 on success, -ENOENT if child cannot be found,
> + * -ENOMEM on malloc failure
> + */
> +int fat_get_name(struct dentry *parent, char *name,
> +		 struct dentry *child)
> +{
> +	struct super_block *sb = parent->d_inode->i_sb;
> +
> +	loff_t child_loc = MSDOS_I(child->d_inode)->i_pos;
> +	int err;
> +
> +	lock_super(sb);
> +
> +	if (child_loc) {
> +		err = fat_name_for_ipos(parent->d_inode, name, NAME_MAX+1,
> +					child_loc);
> +	} else {
> +		child_loc = MSDOS_I(child->d_inode)->i_logstart;
> +		err = fat_name_for_logstart(parent->d_inode, name, NAME_MAX+1,
> +					    child_loc);
> +	}
> +
> +	unlock_super(sb);
> +	return err;
> +}

Please don't add new lock_super() usage if it is not necessary. Almost
all of lock_super() just replaced lock_kernel() usage. It rather should
be removed in future.  Probably, this should use inode->i_mutex instead.

BTW, the above issue is same with all of directory read.

And although this is using i_pos, is there no possibility to be passed
the detached inode (i.e. open but unlinked inode, i_pos == 0)?

What happens in the case of detached inode?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
--
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/