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 17:02:57 +0200
From:	Nick Piggin <npiggin@...e.de>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	linux-fsdevel@...r.kernel.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 10:58:20AM -0400, Christoph Hellwig wrote:
> 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.

Yeah probably right.

 
> > +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.

Oh OK. Some existing places are testing for it...

 
> > +			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.

That's kind of why I liked it in inode_setattr better.

But if the filesystem defines its own ->setattr, then it could simply
not define a ->setsize and do the right thing in setattr. So this
calling convention seems not too bad.


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