linux-kernel - Re: [PATCH 05/39] whiteout/NFSD: Don't return information about whiteouts to userspace

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: <20100507071808.20028458@notabene.brown>
Date:	Fri, 7 May 2010 07:18:08 +1000
From:	Neil Brown <neilb@...e.de>
To:	Valerie Aurora <vaurora@...hat.com>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Christoph Hellwig <hch@...radead.org>,
	Jan Blunck <jblunck@...e.de>,
	David Woodhouse <dwmw2@...radead.org>,
	linux-nfs@...r.kernel.org, "J. Bruce Fields" <bfields@...ldses.org>
Subject: Re: [PATCH 05/39] whiteout/NFSD: Don't return information about
 whiteouts to userspace

On Thu, 6 May 2010 14:01:51 -0400
Valerie Aurora <vaurora@...hat.com> wrote:

> On Tue, May 04, 2010 at 09:37:31AM +1000, Neil Brown wrote:
> > On Mon,  3 May 2010 16:12:04 -0700
> > Valerie Aurora <vaurora@...hat.com> wrote:
> > 
> > > From: Jan Blunck <jblunck@...e.de>
> > > 
> > > Userspace isn't ready for handling another file type, so silently drop
> > > whiteout directory entries before they leave the kernel.
> > 
> > Feels very intrusive doesn't it....
> > 
> > Have you considered something like the following?
> 
> Hrm, I see how that could be more elegant, but I'd rather avoid yet
> another layer of function pointer passing around.  This code is
> already hard enough to review...

 Yes, the extra indirection is a bit of a negative, but I don't think this
 patch is harder to review than the alternate.
 From a numerical perspective, with this patch you only need to look at the
 various places that ->readdir is called to be sure it is always correct.
 There are about 3.  With the original you need to look at ever filldir
 function.  Jan has found 9.  

 And from a maintainability perspective, I think my approach is safer.  Given
 that there are 9 filldir functions already, the chance that a need will be
 found for another seems good, and the chance that the coder will know to
 check for DT_WHT is a best even.  Conversely if another call to ->readdir
 were added it is likely that nothing would need to be done.

 Of course just counting things doesn't give a completely picture but I think
 it can be indicative.

NeilBrown


> 
> -VAL
> 
> > NeilBrown
> > 
> > diff --git a/fs/readdir.c b/fs/readdir.c
> > index 7723401..4c5b347 100644
> > --- a/fs/readdir.c
> > +++ b/fs/readdir.c
> > @@ -19,10 +19,26 @@
> >  
> >  #include <asm/uaccess.h>
> >  
> > +struct readdir_info {
> > +	filldir_t filler;
> > +	void *data;
> > +};
> > +
> > +static int white_out(void *vrdi, const char *name, int namlen,
> > +		     loff_t offset, u64 ino, unsigned int d_type)
> > +{
> > +	struct readdir_info *rdi = vrdi;
> > +	if (d_type == DT_WHT)
> > +		return 0;
> > +	return rdi->filler(rdi->data, name, namlen, offset, info, d_type);
> > +}
> > +
> >  int vfs_readdir(struct file *file, filldir_t filler, void *buf)
> >  {
> >  	struct inode *inode = file->f_path.dentry->d_inode;
> >  	int res = -ENOTDIR;
> > +	struct readir_info rdi = { filler, buf };
> > +
> >  	if (!file->f_op || !file->f_op->readdir)
> >  		goto out;
> >  
> > @@ -36,7 +52,7 @@ int vfs_readdir(struct file *file, filldir_t filler, void *buf)
> >  
> >  	res = -ENOENT;
> >  	if (!IS_DEADDIR(inode)) {
> > -		res = file->f_op->readdir(file, buf, filler);
> > +		res = file->f_op->readdir(file, &rdi, white_out);
> >  		file_accessed(file);
> >  	}
> >  	mutex_unlock(&inode->i_mutex);

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