linux-kernel - Re: [rfc][patch 3/4] fs: new truncate sequence

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:	Tue, 7 Jul 2009 10:58:20 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Nick Piggin <npiggin@...e.de>
Cc:	linux-fsdevel@...r.kernel.org,
	Christoph Hellwig <hch@...radead.org>, Jan Kara <jack@...e.cz>,
	LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org
Subject: Re: [rfc][patch 3/4] fs: new truncate sequence

On Tue, Jul 07, 2009 at 04:48:23PM +0200, Nick Piggin wrote:
> Don't know whether it is a good idea to move i_alloc_sem into implementation.
> Maybe it is better to leave it in the caller in this patchset?

Generally moving locks without changing prototypes can be a very subtle
way to break things.  A good option is to move the locking in a separate
patch set in a patch series or at least release if it's otherwise to
complicated.

> +int simple_setsize(struct dentry *dentry, loff_t newsize,
> +			unsigned flags, struct file *file)

This one could probably also use kerneldoc comment.

> +{
> +	struct inode *inode = dentry->d_inode;
> +	loff_t oldsize;
> +	int error;
> +
> +	error = inode_newsize_ok(inode, newsize);
> +	if (error)
> +		return error;
> +
> +	oldsize = inode->i_size;
> +	i_size_write(inode, newsize);
> +	truncate_pagecache(inode, oldsize, newsize);
> +
> +	return error;
> +}

> +	if (ia_valid & ATTR_SIZE) {
> +		if (inode->i_op && inode->i_op->setsize) {

inode->i_op is mandatory these days.

> +			unsigned int flags = 0;
> +			struct file *file = NULL;
> +
> +			if (ia_valid & ATTR_FILE) {
> +				flags |= SETSIZE_FILE;
> +				file = attr->ia_file;
> +			}
> +			if (ia_valid & ATTR_OPEN)
> +				flags |= SETSIZE_OPEN;
> +			error = inode->i_op->setsize(dentry, attr->ia_size,
> +							flags, file);
> +			if (error)
> +				return error;

So you still pass down to ->setattr if ->setsize succeeded?  That's a
very confusing calling convention.  It also means we first do the
truncation and any following time/mode updates are in a separate
transaction which is a no-go.

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