linux-kernel - Re: [RFC] persistent store (version 3) (part 1 of 2)

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: <20101209131452.GA2260@a1.tnic>
Date:	Thu, 9 Dec 2010 14:14:52 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	"Luck, Tony" <tony.luck@...el.com>
Cc:	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	tglx@...utronix.de, mingo@...e.hu, greg@...ah.com,
	akpm@...ux-foundation.org, ying.huang@...el.com,
	David Miller <davem@...emloft.net>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Kyungmin Park <kmpark@...radead.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [RFC] persistent store (version 3) (part 1 of 2)

On Wed, Dec 08, 2010 at 03:35:40PM -0800, Luck, Tony wrote:
> > > what happens if it IS_ERR?
> > 
> > Good point - I need to clean up if things fail (and akpm would
> > like to see me re-factor so that there is only one "return"
> > statement for better maintainabiliy).  I'll fix up stuff here.
> 
> Here's what I'll have in the next version. With all the error
> handling the pstore_mkfile() routine was getting a bit big, so
> I split out the writing-to-the-file part into another function.
> The pair of functions now look like this:
> 
> /*
>  * Set up a file structure as if we had opened this file and
>  * write our data to it.
>  */
> static int pstore_writefile(struct inode *inode, struct dentry *dentry,
> 	char *data, size_t size)
> {
> 	struct file f;
> 	ssize_t n;
> 	mm_segment_t old_fs = get_fs();
> 
> 	memset(&f, '0', sizeof f);
> 	f.f_mapping = inode->i_mapping;
> 	f.f_path.dentry = dentry;
> 	f.f_path.mnt = pstore_mnt;
> 	f.f_pos = 0;
> 	f.f_op = inode->i_fop;
> 	set_fs(KERNEL_DS);
> 	n = do_sync_write(&f, data, size, &f.f_pos);
> 	set_fs(old_fs);
> 
> 	return n == size;
> }

Good.

> 
> /*
>  * Make a regular file in the root directory of our file system.
>  * Load it up with "size" bytes of data from "buf".
>  * Set the mtime & ctime to the date that this record was originally stored.
>  */
> int pstore_mkfile(char *name, char *data, size_t size, struct timespec time,
> 		  void *private)
> {
> 	struct dentry	*root = pstore_sb->s_root;
> 	struct dentry	*dentry;
> 	struct inode	*inode;
> 	int		rc = 0;
> 
> 	inode = pstore_get_inode(pstore_sb, root->d_inode, S_IFREG | 0444, 0);
> 	if (!inode) {
> 		rc = -ENOMEM;
> 		goto fail;
> 	}
> 	inode->i_private = private;
> 
> 	mutex_lock(&root->d_inode->i_mutex);
> 
> 	dentry = d_alloc_name(root, name);
> 	if (IS_ERR(dentry)) {
> 		mutex_unlock(&root->d_inode->i_mutex);
> 		rc = -ENOSPC;
> 		goto fail1;
> 	}
> 
> 	d_add(dentry, inode);
> 
> 	mutex_unlock(&root->d_inode->i_mutex);
> 
> 	if (!pstore_writefile(inode, dentry, data, size)) {
> 		inode->i_nlink--;
> 		mutex_lock(&root->d_inode->i_mutex);
> 		d_delete(dentry);
> 		dput(dentry);
> 		mutex_unlock(&root->d_inode->i_mutex);
> 		rc = -ENOSPC;
> 		goto fail;
> 	}

don't we have to iput() the inode here too if pstore_writefile() fails?

> 
> 	if (time.tv_sec)
> 		inode->i_mtime = inode->i_ctime = time;
> 
> 	return 0;
> 
> fail1:
> 	iput(inode);
> fail:
> 	return rc;
> }

So this version is ok. However, I'd go and move the error paths to
labels near the end of the function since this makes the main part of
what it does more readable, IMHO. IOW, something like this (untested, of
course):

int pstore_mkfile(char *name, char *data, size_t size, struct timespec time,
                  void *private)
{
        struct dentry   *root = pstore_sb->s_root;
        struct dentry   *dentry;
        struct inode    *inode;
        int             rc;

	rc = -ENOMEM;
        inode = pstore_get_inode(pstore_sb, root->d_inode, S_IFREG | 0444, 0);
        if (!inode)
                goto fail;

        inode->i_private = private;

        mutex_lock(&root->d_inode->i_mutex);

	rc = -ENOSPC;
        dentry = d_alloc_name(root, name);
        if (IS_ERR(dentry))
                goto fail_alloc;

        d_add(dentry, inode);

        mutex_unlock(&root->d_inode->i_mutex);

        if (!pstore_writefile(inode, dentry, data, size))
		goto fail_write;

        if (time.tv_sec)
                inode->i_mtime = inode->i_ctime = time;

        return 0;

fail_write:
	inode->i_nlink--;
	mutex_lock(&root->d_inode->i_mutex);
	d_delete(dentry);
        dput(dentry);
	/* we fall through to unlock the mutex below */

fail_alloc:
	mutex_unlock(&root->d_inode->i_mutex);
        iput(inode);

fail:
        return rc;
}


Hmm.

-- 
Regards/Gruss,
Boris.
--
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/