linux-kernel - Re: [PATCH 1/5] VFS: DazukoFS, stackable-fs, file access control

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]

Date:	Sun, 21 Dec 2008 18:56:46 +0100
From:	John Ogness <dazukocode@...ess.net>
To:	Bastian Blank <bastian@...di.eu.org>
Cc:	linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk,
	malware-list@...ts.printk.net, eparis@...hat.com,
	hch@...radead.org, alan@...rguk.ukuu.org.uk
Subject: Re: [PATCH 1/5] VFS: DazukoFS, stackable-fs, file access control

On 2008-12-21, Bastian Blank <bastian@...di.eu.org> wrote:
> There are several (okay, all) includes missing.

Indeed. I will fix this.

>> +static inline
>> +struct dazukofs_sb_info *GET_SB_INFO(struct super_block *upper_sb)
>
> Coding-style.

The functions were originally macros. The capitalization was inspired
by macros such as IS_ERR, PTR_ERR, MKDEV, etc. That is the
explanation, but it doesn't make it ok. I will change it.

>> +static inline void SET_LOWER_INODE(struct inode *upper_inode,
>> +				   struct inode *lower_inode)
>> +{
>> +	((struct dazukofs_inode_info *)
>> +	 container_of(upper_inode, struct dazukofs_inode_info,
>> +		      vfs_inode))->lower_inode = lower_inode;
>> +}
>
> Please make such cast cascades explicit:
>
> | struct dazukofs_inode_info *info = container_of(...);
> | info->lower_inode = lower_inode;
>
> There are other people which want to read the code.

Agreed.

>> +static int dazukofs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
>> +{
>> +	struct vfsmount *lower_mnt;
>> +	struct dentry *lower_dentry;
>> +	struct vfsmount *vfsmount_save;
>> +	struct dentry *dentry_save;
>> +	int valid;
>> +
>> +	valid = 1;
>> +
>> +	lower_dentry = GET_LOWER_DENTRY(dentry);
>> +
>> +	if (!lower_dentry->d_op || !lower_dentry->d_op->d_revalidate)
>> +		goto out;
>
> Why do you use goto instead of return?

I use a goto here so that only 1 exit point exists within the
function. However, that probably doesn't make much sense when such
basic sanity checks are performed at the beginning of the function. I
will change this.

>> +static int dazukofs_d_hash(struct dentry *dentry, struct qstr *name)
>> +{
>> +	struct dentry *lower_dentry = GET_LOWER_DENTRY(dentry);
>> +
>> +	if (!lower_dentry || !lower_dentry->d_op ||
>> +	    !lower_dentry->d_op->d_hash) {
>> +		return 0;
>> +	}
>
> You mix rather different coding styles through the whole code.

When I wrote the stacking code, it was the first time I started using
the Linux coding style. I will go back through and see what I can
clean up.

> Also why do you say that lower_dentry can be 0 in _hash, but not in
> _revalidate?

_hash doesn't need this check. I will remove it.

>> +static void dazukofs_d_release(struct dentry *dentry)
>> +{
>> +	if (GET_DENTRY_INFO(dentry)) {
>> +		dput(GET_LOWER_DENTRY(dentry));
>> +		mntput(GET_LOWER_MNT(dentry));
>
> Why do you push anything out in other functions while making it
> explicit would make it much easier readable?

For me, writing a stackable filesystem was quite complex.  Using the
all-caps functions for upper/lower layer translations helped me to
easily see what I was doing.  I agree that the all-caps thing is quite
ugly. I will change that, but I would prefer to keep the translation
functions as separate inline functions. (The same technique can be
seen in ecryptfs as well.)

John Ogness
--
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/