linux-kernel - Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}

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: <20061013200705.GB31928@filer.fsl.cs.sunysb.edu>
Date:	Fri, 13 Oct 2006 16:07:05 -0400
From:	Josef Sipek <jsipek@....cs.sunysb.edu>
To:	Andrew Morton <akpm@...l.org>
Cc:	linux-kernel@...r.kernel.org, torvalds@...l.org, hch@...radead.org,
	viro@....linux.org.uk, linux-fsdevel@...r.kernel.org,
	penberg@...helsinki.fi, ezk@...sunysb.edu, mhalcrow@...ibm.com
Subject: Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*

On Fri, Oct 13, 2006 at 12:27:06PM -0700, Andrew Morton wrote:
> On Fri, 13 Oct 2006 07:18:49 -0400
> Josef "Jeff" Sipek <jsipek@...sunysb.edu> wrote:
> > include/linux/stack_fs.h |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> The name stack_fs implies that there's a filesystem called stackfs.  Only
> there isn't.  I wonder if we can choose a better name for all of this. 
> Maybe fs_stack_*?
 
fs_stack_* sounds good to me.
 
> What are the locking requirements for these functions?  Presumably the
> caller must hold i_mutex on at least the source inode, and perhaps the
> destination one?
> 
> If i_mutex is held, i_size_read() isn't needed.
> 
> If i_mutex is held, i_size_write() isn't needed either.
> 
> So please document the locking requirements via source comments and then
> see if this can be simplified.

Fair enough. I'll submit an updated version.

> > +static inline void __stackfs_copy_attr_all(struct inode *dest,
> > +					   const struct inode *src,
> > +					   int (*get_nlinks)(struct inode *))
> > +{
> > +	if (!get_nlinks)
> > +		dest->i_nlink = src->i_nlink;
> > +	else
> > +		dest->i_nlink = get_nlinks(dest);
> 
> I cannot find a get_nlinks() in 2.6.19-rc2?
 
It is the last argument to the function. Perhaps the function name is
deceiving.
 
> Many of these functions are too large to be inlined.  Suggest they be
> placed in fs/fs-stack.c (or whatever we call it).
 
Ack. As a rule of thumb, for functions like these (laundry list of
assignments), what's a good threshold?
 
> The functions themselves seem a bit arbitrary.
> stackfs_copy_attr_timesizes() copy the three timestamps and the size.  Is
> there actually any methodical reason for that, or is it simply some
> sequence which happens to have been observed in ecryptfs?

These functions come from the FiST templates [1]. Some of these can
definitely removed, or split up.

> And please - if I asked these questions when reviewing the patch, others
> will ask them when reading the code two years from now.  So please treat my
> questions as "gosh, I should have put a comment in there".

That's exactly how I look at it.

Josef "Jeff" Sipek.

-- 
Bad pun of the week: The formula 1 control computer suffered from a race
condition
-
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/